code review 87810048: compiler: Use backend interface for defining global dec...

compiler: Use backend interface for defining global declarations.

[cmang, 2014-04-16 20:15:39]

Hello iant@golang.org (cc: gofrontend-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/gofrontend

[cmang, 2014-04-16 20:25:50]

On 2014/04/16 20:15:39, cmang wrote:
> Hello mailto:iant@golang.org (cc: mailto:gofrontend-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/gofrontend

Diff of gcc/go/go-gcc.cc:

Index: gcc/go/go-gcc.cc
======================================================================
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -29,9 +29,11 @@
 #include "stor-layout.h"
 #include "varasm.h"
 #include "tree-iterator.h"
+#include "cgraph.h"
 #include "convert.h"
 #include "basic-block.h"
 #include "gimple-expr.h"
+#include "gimplify.h"
 #include "toplev.h"
 #include "output.h"
 #include "real.h"
@@ -227,6 +229,10 @@ class Gcc_backend : public Backend
   indirect_expression(Bexpression* expr, bool known_valid, Location);
 
   Bexpression*
+  named_constant_expression(Btype* btype, const std::string& name,
+                            Bexpression* val, Location);
+
+  Bexpression*
   integer_constant_expression(Btype* btype, mpz_t val);
 
   Bexpression*
@@ -416,6 +422,22 @@ class Gcc_backend : public Backend
   bool
   function_set_body(Bfunction* function, Bstatement* code_stmt);
 
+  bool
+  push_function_context(Bfunction* function, Location start_location,
+                        Location end_location);
+
+  bool
+  pop_function_context();
+
+  bool
+  process_function(Bfunction* function, bool finalize);
+
+  void
+  write_global_definitions(const std::vector<Btype*>&,
+                           const std::vector<Bexpression*>&,
+                           const std::vector<Bfunction*>&,
+                           const std::vector<Bvariable*>&);
+
  private:
   // Make a Bexpression from a tree.
   Bexpression*
@@ -935,6 +957,8 @@ Gcc_backend::zero_expression(Btype* btype)
     ret = error_mark_node;
   else
     ret = build_zero_cst(t);
+
+  TREE_CONSTANT(ret) = 1;
   return tree_to_expr(ret);
 }
 
@@ -962,6 +986,30 @@ Gcc_backend::indirect_expression(Bexpression* expr, bool
known_valid,
   return tree_to_expr(ret);
 }
 
+
+// Return an expression that declares a constant named NAME with the
+// constant value VAL in BTYPE.
+
+Bexpression*
+Gcc_backend::named_constant_expression(Btype* btype, const std::string& name,
+                                       Bexpression* val, Location location)
+{
+  tree type_tree = btype->get_tree();
+  tree const_val = val->get_tree();
+  if (type_tree == error_mark_node || const_val == error_mark_node)
+    return this->error_expression();
+
+  tree name_tree = get_identifier_from_string(name);
+  tree decl = build_decl(location.gcc_location(), CONST_DECL, name_tree,
+                         type_tree);
+  DECL_INITIAL(decl) = const_val;
+  TREE_CONSTANT(decl) = 1;
+  TREE_READONLY(decl) = 1;
+
+  go_preserve_from_gc(decl);
+  return this->make_expression(decl);
+}
+
 // Return a typed value as a constant integer.
 
 Bexpression*
@@ -2250,8 +2298,6 @@ Gcc_backend::immutable_struct(const std::string& name,
bool is_hidden,
 			 get_identifier_from_string(name),
 			 build_qualified_type(type_tree, TYPE_QUAL_CONST));
   TREE_STATIC(decl) = 1;
-  TREE_READONLY(decl) = 1;
-  TREE_CONSTANT(decl) = 1;
   TREE_USED(decl) = 1;
   DECL_ARTIFICIAL(decl) = 1;
   if (!is_hidden)
@@ -2292,6 +2338,9 @@ Gcc_backend::immutable_struct_set_init(Bvariable* var,
const std::string&,
   if (decl == error_mark_node || init_tree == error_mark_node)
     return;
 
+  TREE_READONLY(decl) = TREE_READONLY(init_tree);
+  TREE_CONSTANT(decl) = TREE_CONSTANT(init_tree);
+
   DECL_INITIAL(decl) = init_tree;
 
   // Now that DECL_INITIAL is set, we can't call make_decl_one_only.
@@ -2511,6 +2560,139 @@ Gcc_backend::function_set_body(Bfunction* function,
Bstatement* code_stmt)
   return true;
 }
 
+// Pushes the function FUNCTION onto this backend's call stack.  Returns
+// true on success, false on failure.
+
+bool
+Gcc_backend::push_function_context(Bfunction* function, Location
start_location,
+                                   Location end_location)
+{
+  tree decl = function->get_tree();
+  if (decl == error_mark_node)
+    return false;
+
+  if (DECL_STRUCT_FUNCTION(decl) == NULL)
+    push_struct_function(decl);
+  else
+    push_cfun(DECL_STRUCT_FUNCTION(decl));
+  cfun->function_start_locus = start_location.gcc_location();
+  cfun->function_end_locus = end_location.gcc_location();
+
+  return true;
+}
+
+// Pops the current function off of this backend's call stack.  A backend
+// function must be pushed onto the stack before being popped off.
+// Returns true on success, false on failure.
+
+bool
+Gcc_backend::pop_function_context()
+{
+  if (current_function_decl == NULL)
+     return false;
+
+  pop_cfun();
+  return true;
+}
+
+// Process the backend representation for the function FUNCTION.
+// FINALIZE is true if the function's backend representation should be
+// finalized.  Returns true on success, false on failure.
+
+bool
+Gcc_backend::process_function(Bfunction* function, bool finalize)
+{
+  tree decl = function->get_tree();
+  if (decl == error_mark_node)
+    return false;
+
+  gimplify_function_tree(decl);
+
+  if (finalize)
+    cgraph_finalize_function(decl, true);
+  else
+    cgraph_add_new_function(decl, false);
+  return true;
+}
+
+// Write the definitions for all TYPE_DECLS, CONSTANT_DECLS,
+// FUNCTION_DECLS, and VARIABLE_DECLS declared globally.
+
+void
+Gcc_backend::write_global_definitions(
+    const std::vector<Btype*>& type_decls,
+    const std::vector<Bexpression*>& constant_decls,
+    const std::vector<Bfunction*>& function_decls,
+    const std::vector<Bvariable*>& variable_decls)
+{
+  size_t count_definitions = type_decls.size() + constant_decls.size()
+      + function_decls.size() + variable_decls.size();
+
+  tree* defs = new tree[count_definitions];
+
+  // Convert all non-erroneous declarations into Gimple form.
+  size_t i = 0;
+  for (std::vector<Bvariable*>::const_iterator p = variable_decls.begin();
+       p != variable_decls.end();
+       ++p)
+    {
+      if ((*p)->get_tree() != error_mark_node)
+        {
+          defs[i] = (*p)->get_tree();
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+
+  for (std::vector<Btype*>::const_iterator p = type_decls.begin();
+       p != type_decls.end();
+       ++p)
+    {
+      tree type_tree = (*p)->get_tree();
+      if (type_tree != error_mark_node
+          && IS_TYPE_OR_DECL_P(type_tree))
+        {
+          defs[i] = TYPE_NAME(type_tree);
+          gcc_assert(defs[i] != NULL);
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+  for (std::vector<Bexpression*>::const_iterator p = constant_decls.begin();
+       p != constant_decls.end();
+       ++p)
+    {
+      if ((*p)->get_tree() != error_mark_node)
+        {
+          defs[i] = (*p)->get_tree();
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+  for (std::vector<Bfunction*>::const_iterator p = function_decls.begin();
+       p != function_decls.end();
+       ++p)
+    {
+      if ((*p)->get_tree() != error_mark_node)
+        {
+          defs[i] = (*p)->get_tree();
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+
+  // Pass everything back to the middle-end.
+
+  wrapup_global_declarations(defs, i);
+
+  finalize_compilation_unit();
+
+  check_global_declarations(defs, i);
+  emit_debug_global_declarations(defs, i);
+
+  delete[] defs;
+}
+
 // The single backend.
 
 static Gcc_backend gcc_backend;

[iant, 2014-04-16 20:53:54]

https://codereview.appspot.com/87810048/diff/60001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/87810048/diff/60001/go/backend.h#newcode660
go/backend.h:660: push_function_context(Bfunction* function, Location
start_location,
We shouldn't need this in the backend interface.  This is how GCC works, but
when we're converting to the backend representation we should only ever have one
function to consider: the one being converted.  So there is no need to push or
pop anything.  And there should be no need to set a function context or
anything.

https://codereview.appspot.com/87810048/diff/60001/go/backend.h#newcode673
go/backend.h:673: process_function(Bfunction* function, bool finalize) = 0;
Can we just call this from write_global_definitions?

The finalize parameter is there for the initialization function.  It's different
because it's currently built entirely in the backend representation.  I wonder
if we could instead build it as a Go function (a separate CL for that, I think).

If there is some reason that it can't be a Go function, then I think there
should probably be a separate backend method for it.  The finalize parameter
here doesn't make much sense.

[iant, 2014-04-16 21:06:13]

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc
File go/expressions.cc (left):

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc#oldcode4664
go/expressions.cc:4664: error_at(location, "constant addition overflow");
You need to sync to the current sources.

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc
File go/expressions.cc (right):

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc#newcode3214
go/expressions.cc:3214: return type->is_basic_type() &&
expr_type->is_basic_type();
I think you also need to check this->expr_->is_immutable().  Something like
    if (!this->expr_->is_immutable())
        return false;

    if (Type::are_identical(type, expr_type, false, NULL))
        return true;

    return type->is_basic_type() && expr_type->is_basic_type();

[cmang, 2014-04-16 22:50:33]

New diff to gcc/go/go-gcc:

Index: gcc/go/go-gcc.cc
======================================================================
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -29,9 +29,11 @@
 #include "stor-layout.h"
 #include "varasm.h"
 #include "tree-iterator.h"
+#include "cgraph.h"
 #include "convert.h"
 #include "basic-block.h"
 #include "gimple-expr.h"
+#include "gimplify.h"
 #include "toplev.h"
 #include "output.h"
 #include "real.h"
@@ -227,6 +229,10 @@ class Gcc_backend : public Backend
   indirect_expression(Bexpression* expr, bool known_valid, Location);
 
   Bexpression*
+  named_constant_expression(Btype* btype, const std::string& name,
+                            Bexpression* val, Location);
+
+  Bexpression*
   integer_constant_expression(Btype* btype, mpz_t val);
 
   Bexpression*
@@ -416,6 +422,20 @@ class Gcc_backend : public Backend
   bool
   function_set_body(Bfunction* function, Bstatement* code_stmt);
 
+  bool
+  set_function_context(Bfunction* function, Location start_location,
+                       Location end_location);
+
+  bool
+  write_init_function(Bfunction* function, Location start_location,
+                      Location end_location);
+
+  void
+  write_global_definitions(const std::vector<Btype*>&,
+                           const std::vector<Bexpression*>&,
+                           const std::vector<Bfunction*>&,
+                           const std::vector<Bvariable*>&);
+
  private:
   // Make a Bexpression from a tree.
   Bexpression*
@@ -962,6 +982,30 @@ Gcc_backend::indirect_expression(Bexpression* expr, bool
known_valid,
   return tree_to_expr(ret);
 }
 
+
+// Return an expression that declares a constant named NAME with the
+// constant value VAL in BTYPE.
+
+Bexpression*
+Gcc_backend::named_constant_expression(Btype* btype, const std::string& name,
+                                       Bexpression* val, Location location)
+{
+  tree type_tree = btype->get_tree();
+  tree const_val = val->get_tree();
+  if (type_tree == error_mark_node || const_val == error_mark_node)
+    return this->error_expression();
+
+  tree name_tree = get_identifier_from_string(name);
+  tree decl = build_decl(location.gcc_location(), CONST_DECL, name_tree,
+                         type_tree);
+  DECL_INITIAL(decl) = const_val;
+  TREE_CONSTANT(decl) = 1;
+  TREE_READONLY(decl) = 1;
+
+  go_preserve_from_gc(decl);
+  return this->make_expression(decl);
+}
+
 // Return a typed value as a constant integer.
 
 Bexpression*
@@ -2250,8 +2294,6 @@ Gcc_backend::immutable_struct(const std::string& name,
bool is_hidden,
 			 get_identifier_from_string(name),
 			 build_qualified_type(type_tree, TYPE_QUAL_CONST));
   TREE_STATIC(decl) = 1;
-  TREE_READONLY(decl) = 1;
-  TREE_CONSTANT(decl) = 1;
   TREE_USED(decl) = 1;
   DECL_ARTIFICIAL(decl) = 1;
   if (!is_hidden)
@@ -2292,6 +2334,9 @@ Gcc_backend::immutable_struct_set_init(Bvariable* var,
const std::string&,
   if (decl == error_mark_node || init_tree == error_mark_node)
     return;
 
+  TREE_READONLY(decl) = TREE_READONLY(init_tree);
+  TREE_CONSTANT(decl) = TREE_CONSTANT(init_tree);
+
   DECL_INITIAL(decl) = init_tree;
 
   // Now that DECL_INITIAL is set, we can't call make_decl_one_only.
@@ -2511,6 +2556,137 @@ Gcc_backend::function_set_body(Bfunction* function,
Bstatement* code_stmt)
   return true;
 }
 
+// Set the backend's function context to FUNCTION.  Returns true on success,
+// false on failure.
+
+bool
+Gcc_backend::set_function_context(Bfunction* function, Location start_location,
+                                  Location end_location)
+{
+  tree decl = function->get_tree();
+  if (decl == error_mark_node)
+    return false;
+
+  if (current_function_decl != NULL)
+    pop_cfun();
+
+  if (DECL_STRUCT_FUNCTION(decl) == NULL)
+    push_struct_function(decl);
+  else
+    push_cfun(DECL_STRUCT_FUNCTION(decl));
+  cfun->function_start_locus = start_location.gcc_location();
+  cfun->function_end_locus = end_location.gcc_location();
+
+  return true;
+}
+
+// Create the initialization function FUNCTION.  Returns true on success, false
+// on failure.
+
+bool
+Gcc_backend::write_init_function(Bfunction* function, Location start_location,
+                                 Location end_location)
+{
+  tree decl = function->get_tree();
+  if (decl == error_mark_node)
+    return false;
+
+  if (DECL_STRUCT_FUNCTION(decl) == NULL)
+    push_struct_function(decl);
+  else
+    push_cfun(DECL_STRUCT_FUNCTION(decl));
+  cfun->function_start_locus = start_location.gcc_location();
+  cfun->function_end_locus = end_location.gcc_location();
+
+  gimplify_function_tree(decl);
+  cgraph_add_new_function(decl, false);
+
+  pop_cfun();
+  return true;
+}
+
+// Write the definitions for all TYPE_DECLS, CONSTANT_DECLS,
+// FUNCTION_DECLS, and VARIABLE_DECLS declared globally.
+
+void
+Gcc_backend::write_global_definitions(
+    const std::vector<Btype*>& type_decls,
+    const std::vector<Bexpression*>& constant_decls,
+    const std::vector<Bfunction*>& function_decls,
+    const std::vector<Bvariable*>& variable_decls)
+{
+  size_t count_definitions = type_decls.size() + constant_decls.size()
+      + function_decls.size() + variable_decls.size();
+
+  tree* defs = new tree[count_definitions];
+
+  // Convert all non-erroneous declarations into Gimple form.
+  size_t i = 0;
+  for (std::vector<Bvariable*>::const_iterator p = variable_decls.begin();
+       p != variable_decls.end();
+       ++p)
+    {
+      if ((*p)->get_tree() != error_mark_node)
+        {
+          defs[i] = (*p)->get_tree();
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+
+  for (std::vector<Btype*>::const_iterator p = type_decls.begin();
+       p != type_decls.end();
+       ++p)
+    {
+      tree type_tree = (*p)->get_tree();
+      if (type_tree != error_mark_node
+          && IS_TYPE_OR_DECL_P(type_tree))
+        {
+          defs[i] = TYPE_NAME(type_tree);
+          gcc_assert(defs[i] != NULL);
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+  for (std::vector<Bexpression*>::const_iterator p = constant_decls.begin();
+       p != constant_decls.end();
+       ++p)
+    {
+      if ((*p)->get_tree() != error_mark_node)
+        {
+          defs[i] = (*p)->get_tree();
+          go_preserve_from_gc(defs[i]);
+          ++i;
+        }
+    }
+  for (std::vector<Bfunction*>::const_iterator p = function_decls.begin();
+       p != function_decls.end();
+       ++p)
+    {
+      tree decl = (*p)->get_tree();
+      if (decl != error_mark_node)
+        {
+          go_preserve_from_gc(decl);
+          gimplify_function_tree(decl);
+          cgraph_finalize_function(decl, true);
+
+          defs[i] = decl;
+          ++i;
+        }
+    }
+
+  // Pass everything back to the middle-end.
+
+  wrapup_global_declarations(defs, i);
+
+  finalize_compilation_unit();
+
+  check_global_declarations(defs, i);
+  emit_debug_global_declarations(defs, i);
+
+  delete[] defs;
+}
+
 // The single backend.
 
 static Gcc_backend gcc_backend;

https://codereview.appspot.com/87810048/diff/60001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/87810048/diff/60001/go/backend.h#newcode660
go/backend.h:660: push_function_context(Bfunction* function, Location
start_location,
On 2014/04/16 20:53:54, iant wrote:
> We shouldn't need this in the backend interface.  This is how GCC works, but
> when we're converting to the backend representation we should only ever have
one
> function to consider: the one being converted.  So there is no need to push or
> pop anything.  And there should be no need to set a function context or
> anything.

Replaced with set_function_context.

https://codereview.appspot.com/87810048/diff/60001/go/backend.h#newcode673
go/backend.h:673: process_function(Bfunction* function, bool finalize) = 0;
On 2014/04/16 20:53:54, iant wrote:
> Can we just call this from write_global_definitions?
> 
> The finalize parameter is there for the initialization function.  It's
different
> because it's currently built entirely in the backend representation.  I wonder
> if we could instead build it as a Go function (a separate CL for that, I
think).
> 
> If there is some reason that it can't be a Go function, then I think there
> should probably be a separate backend method for it.  The finalize parameter
> here doesn't make much sense.

Done.

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc
File go/expressions.cc (left):

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc#oldcode4664
go/expressions.cc:4664: error_at(location, "constant addition overflow");
On 2014/04/16 21:06:13, iant wrote:
> You need to sync to the current sources.

Done.

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc
File go/expressions.cc (right):

https://codereview.appspot.com/87810048/diff/60001/go/expressions.cc#newcode3214
go/expressions.cc:3214: return type->is_basic_type() &&
expr_type->is_basic_type();
On 2014/04/16 21:06:13, iant wrote:
> I think you also need to check this->expr_->is_immutable().  Something like
>     if (!this->expr_->is_immutable())
>         return false;
> 
>     if (Type::are_identical(type, expr_type, false, NULL))
>         return true;
> 
>     return type->is_basic_type() && expr_type->is_basic_type();

Done.

[iant, 2014-04-17 15:29:53]

Might be worth splitting out the changes to expressions.cc into a smaller CL.

Might also be worth rewriting the way the initialization function is handled in
a different CL.  Change it from being entirely backend to being a regular Go
function, albeit a generated one.

https://codereview.appspot.com/87810048/diff/90001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/87810048/diff/90001/go/backend.h#newcode658
go/backend.h:658: // success, false on failure.
I looked at the parameter_variable and local_variable backend methods, and I see
that they already take a function argument, and use it to set the DECL_CONTEXT. 
So now I'm wondering whether we need to do this at all.  What goes wrong if we
don't have a method like this?

https://codereview.appspot.com/87810048/diff/90001/go/backend.h#newcode666
go/backend.h:666: write_init_function(Bfunction* initfn, Location
start_location,
What if we add the initialization function to the list we pass to
write_global_definitions?  If we do that, do we still need a function like this?

https://codereview.appspot.com/87810048/diff/90001/go/gogo.cc
File go/gogo.cc (right):

https://codereview.appspot.com/87810048/diff/90001/go/gogo.cc#newcode1178
go/gogo.cc:1178: if (!this->backend()->set_function_context(init_fndecl, loc,
loc))
We're currently dealing with the initialization function entirely as a backend
entity.  If we instead create a Named_object for it, as we do in places like
Build_recover_thunks::function, then we will have something we can pass to
var->get_init_block.  Then we shouldn't need to call set_function_context here.

https://codereview.appspot.com/87810048/diff/90001/go/gogo.h
File go/gogo.h (right):

https://codereview.appspot.com/87810048/diff/90001/go/gogo.h#newcode2220
go/gogo.h:2220: // The backend representation for this object if it is a
NAMED_OBJECT_TYPE.
I think these can all move into the appropriate elements of u_.  Put
backend_type_ in Named_type (actually I think it is already there anyhow), put
backend_const_ in Named_constant, put backend_function_ in Function (that one is
already there also).

[cmang, 2014-04-17 17:43:03]

FYI, the bulk of the changes to expressions.cc have been split into
https://codereview.appspot.com/88080048/ and
https://codereview.appspot.com/88950043/ as suggested.

[cmang, 2014-04-24 21:39:25]

https://codereview.appspot.com/87810048/diff/90001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/87810048/diff/90001/go/backend.h#newcode658
go/backend.h:658: // success, false on failure.
On 2014/04/17 15:29:53, iant wrote:
> I looked at the parameter_variable and local_variable backend methods, and I
see
> that they already take a function argument, and use it to set the
DECL_CONTEXT. 
> So now I'm wondering whether we need to do this at all.  What goes wrong if we
> don't have a method like this?

Done.

https://codereview.appspot.com/87810048/diff/90001/go/backend.h#newcode666
go/backend.h:666: write_init_function(Bfunction* initfn, Location
start_location,
On 2014/04/17 15:29:53, iant wrote:
> What if we add the initialization function to the list we pass to
> write_global_definitions?  If we do that, do we still need a function like
this?

Done.

https://codereview.appspot.com/87810048/diff/90001/go/gogo.cc
File go/gogo.cc (right):

https://codereview.appspot.com/87810048/diff/90001/go/gogo.cc#newcode1178
go/gogo.cc:1178: if (!this->backend()->set_function_context(init_fndecl, loc,
loc))
On 2014/04/17 15:29:53, iant wrote:
> We're currently dealing with the initialization function entirely as a backend
> entity.  If we instead create a Named_object for it, as we do in places like
> Build_recover_thunks::function, then we will have something we can pass to
> var->get_init_block.  Then we shouldn't need to call set_function_context
here.

Done.

https://codereview.appspot.com/87810048/diff/90001/go/gogo.h
File go/gogo.h (right):

https://codereview.appspot.com/87810048/diff/90001/go/gogo.h#newcode2220
go/gogo.h:2220: // The backend representation for this object if it is a
NAMED_OBJECT_TYPE.
On 2014/04/17 15:29:53, iant wrote:
> I think these can all move into the appropriate elements of u_.  Put
> backend_type_ in Named_type (actually I think it is already there anyhow), put
> backend_const_ in Named_constant, put backend_function_ in Function (that one
is
> already there also).

Done.

[cmang, 2014-04-24 21:42:30]

On 2014/04/24 21:39:25, cmang wrote:
> https://codereview.appspot.com/87810048/diff/90001/go/backend.h
> File go/backend.h (right):
> 
> https://codereview.appspot.com/87810048/diff/90001/go/backend.h#newcode658
> go/backend.h:658: // success, false on failure.
> On 2014/04/17 15:29:53, iant wrote:
> > I looked at the parameter_variable and local_variable backend methods, and I
> see
> > that they already take a function argument, and use it to set the
> DECL_CONTEXT. 
> > So now I'm wondering whether we need to do this at all.  What goes wrong if
we
> > don't have a method like this?
> 
> Done.
> 
> https://codereview.appspot.com/87810048/diff/90001/go/backend.h#newcode666
> go/backend.h:666: write_init_function(Bfunction* initfn, Location
> start_location,
> On 2014/04/17 15:29:53, iant wrote:
> > What if we add the initialization function to the list we pass to
> > write_global_definitions?  If we do that, do we still need a function like
> this?
> 
> Done.
> 
> https://codereview.appspot.com/87810048/diff/90001/go/gogo.cc
> File go/gogo.cc (right):
> 
> https://codereview.appspot.com/87810048/diff/90001/go/gogo.cc#newcode1178
> go/gogo.cc:1178: if (!this->backend()->set_function_context(init_fndecl, loc,
> loc))
> On 2014/04/17 15:29:53, iant wrote:
> > We're currently dealing with the initialization function entirely as a
backend
> > entity.  If we instead create a Named_object for it, as we do in places like
> > Build_recover_thunks::function, then we will have something we can pass to
> > var->get_init_block.  Then we shouldn't need to call set_function_context
> here.
> 
> Done.
> 
> https://codereview.appspot.com/87810048/diff/90001/go/gogo.h
> File go/gogo.h (right):
> 
> https://codereview.appspot.com/87810048/diff/90001/go/gogo.h#newcode2220
> go/gogo.h:2220: // The backend representation for this object if it is a
> NAMED_OBJECT_TYPE.
> On 2014/04/17 15:29:53, iant wrote:
> > I think these can all move into the appropriate elements of u_.  Put
> > backend_type_ in Named_type (actually I think it is already there anyhow),
put
> > backend_const_ in Named_constant, put backend_function_ in Function (that
one
> is
> > already there also).
> 
> Done.

New diff to gcc/go/go-gcc.cc (linked from the playground to avoid going over the
message character limit)
http://play.golang.org/p/pfuPVlOM_d

[iant, 2014-04-25 04:11:07]

I'm nervous about to change to Gcc_backend::immutable_struct to change the value
of TREE_READONLY and TREE_CONSTANT between Gcc_backend::immutable_struct and
Gcc_backend::immutable_struct_set_init.  There have been cases in the recent
past where changing flags led to trouble.  See the long comment about the
setting of DECL_WEAK in Gcc_backend::immutable_struct.

What are the cases where the initializer is not READONLY or CONST?

https://codereview.appspot.com/87810048/diff/210001/go/expressions.cc
File go/expressions.cc (right):

https://codereview.appspot.com/87810048/diff/210001/go/expressions.cc#newcode...
go/expressions.cc:3645: return this->expr_->is_immutable()
Put parentheses around the whole expression, and line up the second line under
"this".

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc
File go/gogo.cc (right):

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode1
go/gogo.cc:1: // Gogo.cc -- Go frontend parsed representation.
This should still just be gogo.cc.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode1238
go/gogo.cc:1238: // Complex variable initializations, first sorting them into a
s/Complex/Complete/ (I know this is pre-existing).

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode6646
go/gogo.cc:6646: const_decls.push_back(gogo->backend()->error_expression());
Doing it this way, I don't think there is any point to pushing error_expression
on const_decls.  Just don't push anything in that case.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode6655
go/gogo.cc:6655: type_decls.push_back(gogo->backend()->error_type());
Similarly, don't push anything for an erroneous name.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode6690
go/gogo.cc:6690: func_decls.push_back(gogo->backend()->error_function());
Don't push anything if erroneous.

[cmang, 2014-04-25 22:34:44]

https://codereview.appspot.com/87810048/diff/210001/go/expressions.cc
File go/expressions.cc (right):

https://codereview.appspot.com/87810048/diff/210001/go/expressions.cc#newcode...
go/expressions.cc:3645: return this->expr_->is_immutable()
On 2014/04/25 04:11:07, iant wrote:
> Put parentheses around the whole expression, and line up the second line under
> "this".

Done.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc
File go/gogo.cc (right):

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode1
go/gogo.cc:1: // Gogo.cc -- Go frontend parsed representation.
On 2014/04/25 04:11:07, iant wrote:
> This should still just be gogo.cc.

Done.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode1238
go/gogo.cc:1238: // Complex variable initializations, first sorting them into a
On 2014/04/25 04:11:07, iant wrote:
> s/Complex/Complete/ (I know this is pre-existing).

Done.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode6646
go/gogo.cc:6646: const_decls.push_back(gogo->backend()->error_expression());
On 2014/04/25 04:11:07, iant wrote:
> Doing it this way, I don't think there is any point to pushing
error_expression
> on const_decls.  Just don't push anything in that case.

Done.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode6655
go/gogo.cc:6655: type_decls.push_back(gogo->backend()->error_type());
On 2014/04/25 04:11:07, iant wrote:
> Similarly, don't push anything for an erroneous name.

Done.

https://codereview.appspot.com/87810048/diff/210001/go/gogo.cc#newcode6690
go/gogo.cc:6690: func_decls.push_back(gogo->backend()->error_function());
On 2014/04/25 04:11:07, iant wrote:
> Don't push anything if erroneous.

Done.

[cmang, 2014-04-25 22:37:13]

On 2014/04/25 04:11:07, iant wrote:
> I'm nervous about to change to Gcc_backend::immutable_struct to change the
value
> of TREE_READONLY and TREE_CONSTANT between Gcc_backend::immutable_struct and
> Gcc_backend::immutable_struct_set_init.  There have been cases in the recent
> past where changing flags led to trouble.  See the long comment about the
> setting of DECL_WEAK in Gcc_backend::immutable_struct.
> 

Agreed. I restored the original immutable_struct logic and instead added
Backend::gc_root_variable since I think this is a special enough case.
Diff of gcc/go/go-gcc.cc: http://play.golang.org/p/9F1zMfnHfV

[iant, 2014-04-26 03:32:44]

LGTM

[iant, 2014-04-26 03:33:04]

*** Submitted as
https://code.google.com/p/gofrontend/source/detail?r=e403fdfd3005 ***

compiler: Use backend interface for defining global declarations.

R=iant
CC=gofrontend-dev
https://codereview.appspot.com/87810048

Committer: Ian Lance Taylor <iant@golang.org>