[PATCH] Handle undefined extern vars in output_in_order

[PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

Hi,

This patch teaches cgraphunit.c:output_in_order to output undefined external
variables via assemble_undefined_decl.  At the moment that is only done for
-ftoplevel-reorder in varpool.c:symbol_table::output_variables.  This patch
makes both behave the same way.  I've also made handling of variables in both
functions look similar to each other.

I'll admit it's rather unclear to me what the '!DECL_EXTERNAL (pv->decl)'
condition is guarding in output_in_order (removed in my patch).  AIUI, it
should be always true for defined variables (or, conversely, if it's false
for a defined variable, why are we skipping it?).

I have plans to reimplement handling of "omp declare target link" in a more
clean manner.  Now it looks quite out of place there.

Bootstrapped and regtested on x86_64, OK for trunk?

Alexander

	* cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF.
	(output_in_order): Loop over undefined variables too.  Output them
	via assemble_undefined_decl.  Skip "omp declare target link" variables
	earlier.
	* varpool.c (symbol_table::output_variables): Handle undefined
	variables together with defined ones.

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 4bfcad7..5a04902 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind
   ORDER_UNDEFINED = 0,
   ORDER_FUNCTION,
   ORDER_VAR,
+  ORDER_VAR_UNDEF,
   ORDER_ASM
 };
 
@@ -2187,16 +2188,25 @@ output_in_order (bool no_reorder)
 	}
     }
 
-  FOR_EACH_DEFINED_VARIABLE (pv)
-    if (!DECL_EXTERNAL (pv->decl))
-      {
-	if (no_reorder && !pv->no_reorder)
-	    continue;
-	i = pv->order;
-	gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
-	nodes[i].kind = ORDER_VAR;
-	nodes[i].u.v = pv;
-      }
+  FOR_EACH_VARIABLE (pv)
+    {
+      if (no_reorder && !pv->no_reorder)
+	continue;
+      if (DECL_HARD_REGISTER (pv->decl))
+	continue;
+      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
+	{
+	  gcc_checking_assert (lookup_attribute ("omp declare target link",
+						 DECL_ATTRIBUTES (pv->decl)));
+#ifdef ACCEL_COMPILER
+	  continue;
+#endif
+	}
+      i = pv->order;
+      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
+      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
+      nodes[i].u.v = pv;
+    }
 
   for (pa = symtab->first_asm_symbol (); pa; pa = pa->next)
     {
@@ -2222,16 +2232,13 @@ output_in_order (bool no_reorder)
 	  break;
 
 	case ORDER_VAR:
-#ifdef ACCEL_COMPILER
-	  /* Do not assemble "omp declare target link" vars.  */
-	  if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl)
-	      && lookup_attribute ("omp declare target link",
-				   DECL_ATTRIBUTES (nodes[i].u.v->decl)))
-	    break;
-#endif
 	  nodes[i].u.v->assemble_decl ();
 	  break;
 
+	case ORDER_VAR_UNDEF:
+	  assemble_undefined_decl (nodes[i].u.v->decl);
+	  break;
+
 	case ORDER_ASM:
 	  assemble_asm (nodes[i].u.a->asm_str);
 	  break;
diff --git a/gcc/varpool.c b/gcc/varpool.c
index ab615fa..b513026 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -733,11 +733,6 @@ symbol_table::output_variables (void)
 
   timevar_push (TV_VAROUT);
 
-  FOR_EACH_VARIABLE (node)
-    if (!node->definition
-	&& !DECL_HAS_VALUE_EXPR_P (node->decl)
- 	&& !DECL_HARD_REGISTER (node->decl))
-      assemble_undefined_decl (node->decl);
   FOR_EACH_DEFINED_VARIABLE (node)
     {
       /* Handled in output_in_order.  */
@@ -747,20 +742,24 @@ symbol_table::output_variables (void)
       node->finalize_named_section_flags ();
     }
 
-  FOR_EACH_DEFINED_VARIABLE (node)
+  FOR_EACH_VARIABLE (node)
     {
-      /* Handled in output_in_order.  */
       if (node->no_reorder)
 	continue;
+      if (DECL_HARD_REGISTER (node->decl))
+	continue;
+      if (DECL_HAS_VALUE_EXPR_P (node->decl))
+	{
+	  gcc_checking_assert (lookup_attribute ("omp declare target link",
+						 DECL_ATTRIBUTES (node->decl)));
 #ifdef ACCEL_COMPILER
-      /* Do not assemble "omp declare target link" vars.  */
-      if (DECL_HAS_VALUE_EXPR_P (node->decl)
-	  && lookup_attribute ("omp declare target link",
-			       DECL_ATTRIBUTES (node->decl)))
-	continue;
+	  continue;
 #endif
-      if (node->assemble_decl ())
-        changed = true;
+	}
+      if (node->definition)
+	changed |= node->assemble_decl ();
+      else
+	assemble_undefined_decl (node->decl);
     }
   timevar_pop (TV_VAROUT);
   return changed;

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

On Thu, 9 Jun 2016, Alexander Monakov wrote:

> Hi,
> 
> This patch teaches cgraphunit.c:output_in_order to output undefined external
> variables via assemble_undefined_decl.  At the moment that is only done for
> -ftoplevel-reorder in varpool.c:symbol_table::output_variables.  This patch
> makes both behave the same way.  I've also made handling of variables in both
> functions look similar to each other.

Ping.

Thanks.
Alexander

Re: [PATCH] Handle undefined extern vars in output_in_order

[Jan Hubicka]

> On Thu, 9 Jun 2016, Alexander Monakov wrote:
> 
> > Hi,
> > 
> > This patch teaches cgraphunit.c:output_in_order to output undefined external
> > variables via assemble_undefined_decl.  At the moment that is only done for
> > -ftoplevel-reorder in varpool.c:symbol_table::output_variables.  This patch
> > makes both behave the same way.  I've also made handling of variables in both
> > functions look similar to each other.
> 
> Ping.
+  FOR_EACH_VARIABLE (pv)
+    {
+      if (no_reorder && !pv->no_reorder)
+	continue;
+      if (DECL_HARD_REGISTER (pv->decl))
+	continue;
+      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
+	{
+	  gcc_checking_assert (lookup_attribute ("omp declare target link",
+						 DECL_ATTRIBUTES (pv->decl)));
+#ifdef ACCEL_COMPILER
+	  continue;
+#endif
+	}
+      i = pv->order;
+      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
+      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
+      nodes[i].u.v = pv;

order for undefined variables is not computed, so it will be 0. Doesn't think overwrite existing
entries of nodes array?

Honza
> 
> Thanks.
> Alexander

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

On Thu, 16 Jun 2016, Jan Hubicka wrote:
> > On Thu, 9 Jun 2016, Alexander Monakov wrote:
> +  FOR_EACH_VARIABLE (pv)
[snip]
> +      i = pv->order;
> +      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
> +      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
> +      nodes[i].u.v = pv;
> 
> order for undefined variables is not computed, so it will be 0. Doesn't
> think overwrite existing entries of nodes array?

Hm, I've tried the following testcase:

extern int a, b;
int f()
{
  return a+b;
}

and in the above loop I see pv->order == 2 on the first iteration,
pv->order == 1 on the second. Under what circumstances wouldn't
order be computed?

Thanks.
Alexander

Re: [PATCH] Handle undefined extern vars in output_in_order

[Jan Hubicka]

> On Thu, 16 Jun 2016, Jan Hubicka wrote:
> > > On Thu, 9 Jun 2016, Alexander Monakov wrote:
> > +  FOR_EACH_VARIABLE (pv)
> [snip]
> > +      i = pv->order;
> > +      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
> > +      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
> > +      nodes[i].u.v = pv;
> > 
> > order for undefined variables is not computed, so it will be 0. Doesn't
> > think overwrite existing entries of nodes array?
> 
> Hm, I've tried the following testcase:
> 
> extern int a, b;
> int f()
> {
>   return a+b;
> }
> 
> and in the above loop I see pv->order == 2 on the first iteration,
> pv->order == 1 on the second. Under what circumstances wouldn't
> order be computed?

I see, order is created at a time variable is added to symbol table (not at time when definition is given).
So we should have order everywhere.
Patch is OK

Honza
> 
> Thanks.
> Alexander

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

On Thu, 16 Jun 2016, Jan Hubicka wrote:
> I see, order is created at a time variable is added to symbol table (not at
> time when definition is given).  So we should have order everywhere.
> Patch is OK

Thanks!  If you don't mind a quick followup question: now that both
FOR_EACH_VARIABLE loops in two functions have the same structure, is
it alright to add a comment of the form

  /* There is a similar loop in output_in_order.  Please keep them in sync.  */

to symbol_table::output_variables, and vice versa?

Alexander

Re: [PATCH] Handle undefined extern vars in output_in_order

[Jan Hubicka]

> On Thu, 16 Jun 2016, Jan Hubicka wrote:
> > I see, order is created at a time variable is added to symbol table (not at
> > time when definition is given).  So we should have order everywhere.
> > Patch is OK
> 
> Thanks!  If you don't mind a quick followup question: now that both
> FOR_EACH_VARIABLE loops in two functions have the same structure, is
> it alright to add a comment of the form
> 
>   /* There is a similar loop in output_in_order.  Please keep them in sync.  */
> 
> to symbol_table::output_variables, and vice versa?

Yes, that is fine.

Honza

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

Hi,

I've discovered that this assert in my patch was too restrictive:

+      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
+	{
+	  gcc_checking_assert (lookup_attribute ("omp declare target link",
+						 DECL_ATTRIBUTES (pv->decl)));

Testing for the nvptx target uncovered that there's another case where a
global variable would have a value expr: emutls.  Sorry for not spotting it
earlier (but at least the new assert did its job).  I think we should always
skip here over decls that have value-exprs, just like hard-reg vars are
skipped.  The following patch does that.  Is this still OK?

(bootstrapped/regtested on x86-64)

Alexander

	* cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF.
	(output_in_order): Loop over undefined variables too.  Output them
	via assemble_undefined_decl.  Skip variables that correspond to hard
	registers or have value-exprs.
	* varpool.c (symbol_table::output_variables): Handle undefined
	variables together with defined ones.
 
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 4bfcad7..e30fe6e 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind
   ORDER_UNDEFINED = 0,
   ORDER_FUNCTION,
   ORDER_VAR,
+  ORDER_VAR_UNDEF,
   ORDER_ASM
 };
 
@@ -2187,16 +2188,20 @@ output_in_order (bool no_reorder)
        }
     }
 
-  FOR_EACH_DEFINED_VARIABLE (pv)
-    if (!DECL_EXTERNAL (pv->decl))
-      {
-       if (no_reorder && !pv->no_reorder)
-           continue;
-       i = pv->order;
-       gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
-       nodes[i].kind = ORDER_VAR;
-       nodes[i].u.v = pv;
-      }
+  /* There is a similar loop in symbol_table::output_variables.
+     Please keep them in sync.  */
+  FOR_EACH_VARIABLE (pv)
+    {
+      if (no_reorder && !pv->no_reorder)
+       continue;
+      if (DECL_HARD_REGISTER (pv->decl)
+         || DECL_HAS_VALUE_EXPR_P (pv->decl))
+       continue;
+      i = pv->order;
+      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
+      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
+      nodes[i].u.v = pv;
+    }
 
   for (pa = symtab->first_asm_symbol (); pa; pa = pa->next)
     {
@@ -2222,16 +2227,13 @@ output_in_order (bool no_reorder)
          break;
 
        case ORDER_VAR:
-#ifdef ACCEL_COMPILER
-         /* Do not assemble "omp declare target link" vars.  */
-         if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl)
-             && lookup_attribute ("omp declare target link",
-                                  DECL_ATTRIBUTES (nodes[i].u.v->decl)))
-           break;
-#endif
          nodes[i].u.v->assemble_decl ();
          break;
 
+       case ORDER_VAR_UNDEF:
+         assemble_undefined_decl (nodes[i].u.v->decl);
+         break;
+
        case ORDER_ASM:
          assemble_asm (nodes[i].u.a->asm_str);
          break;
diff --git a/gcc/varpool.c b/gcc/varpool.c
index ab615fa..e5f991e 100644
--- a/gcc/varpool.c
+++ b/gcc/varpool.c
@@ -733,11 +733,6 @@ symbol_table::output_variables (void)
 
   timevar_push (TV_VAROUT);
 
-  FOR_EACH_VARIABLE (node)
-    if (!node->definition
-       && !DECL_HAS_VALUE_EXPR_P (node->decl)
-       && !DECL_HARD_REGISTER (node->decl))
-      assemble_undefined_decl (node->decl);
   FOR_EACH_DEFINED_VARIABLE (node)
     {
       /* Handled in output_in_order.  */
@@ -747,20 +742,19 @@ symbol_table::output_variables (void)
       node->finalize_named_section_flags ();
     }
 
-  FOR_EACH_DEFINED_VARIABLE (node)
+  /* There is a similar loop in output_in_order.  Please keep them in sync.  */
+  FOR_EACH_VARIABLE (node)
     {
       /* Handled in output_in_order.  */
       if (node->no_reorder)
        continue;
-#ifdef ACCEL_COMPILER
-      /* Do not assemble "omp declare target link" vars.  */
-      if (DECL_HAS_VALUE_EXPR_P (node->decl)
-         && lookup_attribute ("omp declare target link",
-                              DECL_ATTRIBUTES (node->decl)))
+      if (DECL_HARD_REGISTER (node->decl)
+         || DECL_HAS_VALUE_EXPR_P (node->decl))
        continue;
-#endif
-      if (node->assemble_decl ())
-        changed = true;
+      if (node->definition)
+       changed |= node->assemble_decl ();
+      else
+       assemble_undefined_decl (node->decl);
     }
   timevar_pop (TV_VAROUT);
   return changed;

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

On Thu, 23 Jun 2016, Alexander Monakov wrote:
> Hi,
> 
> I've discovered that this assert in my patch was too restrictive:
> 
> +      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
> +	{
> +	  gcc_checking_assert (lookup_attribute ("omp declare target link",
> +						 DECL_ATTRIBUTES (pv->decl)));
> 
> Testing for the nvptx target uncovered that there's another case where a
> global variable would have a value expr: emutls.  Sorry for not spotting it
> earlier (but at least the new assert did its job).  I think we should always
> skip here over decls that have value-exprs, just like hard-reg vars are
> skipped.  The following patch does that.  Is this still OK?

Ping.

> (bootstrapped/regtested on x86-64)
> 
> Alexander
> 
> 	* cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF.
> 	(output_in_order): Loop over undefined variables too.  Output them
> 	via assemble_undefined_decl.  Skip variables that correspond to hard
> 	registers or have value-exprs.
> 	* varpool.c (symbol_table::output_variables): Handle undefined
> 	variables together with defined ones.
>  
> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index 4bfcad7..e30fe6e 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind
>    ORDER_UNDEFINED = 0,
>    ORDER_FUNCTION,
>    ORDER_VAR,
> +  ORDER_VAR_UNDEF,
>    ORDER_ASM
>  };
>  
> @@ -2187,16 +2188,20 @@ output_in_order (bool no_reorder)
>         }
>      }
>  
> -  FOR_EACH_DEFINED_VARIABLE (pv)
> -    if (!DECL_EXTERNAL (pv->decl))
> -      {
> -       if (no_reorder && !pv->no_reorder)
> -           continue;
> -       i = pv->order;
> -       gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
> -       nodes[i].kind = ORDER_VAR;
> -       nodes[i].u.v = pv;
> -      }
> +  /* There is a similar loop in symbol_table::output_variables.
> +     Please keep them in sync.  */
> +  FOR_EACH_VARIABLE (pv)
> +    {
> +      if (no_reorder && !pv->no_reorder)
> +       continue;
> +      if (DECL_HARD_REGISTER (pv->decl)
> +         || DECL_HAS_VALUE_EXPR_P (pv->decl))
> +       continue;
> +      i = pv->order;
> +      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
> +      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
> +      nodes[i].u.v = pv;
> +    }
>  
>    for (pa = symtab->first_asm_symbol (); pa; pa = pa->next)
>      {
> @@ -2222,16 +2227,13 @@ output_in_order (bool no_reorder)
>           break;
>  
>         case ORDER_VAR:
> -#ifdef ACCEL_COMPILER
> -         /* Do not assemble "omp declare target link" vars.  */
> -         if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl)
> -             && lookup_attribute ("omp declare target link",
> -                                  DECL_ATTRIBUTES (nodes[i].u.v->decl)))
> -           break;
> -#endif
>           nodes[i].u.v->assemble_decl ();
>           break;
>  
> +       case ORDER_VAR_UNDEF:
> +         assemble_undefined_decl (nodes[i].u.v->decl);
> +         break;
> +
>         case ORDER_ASM:
>           assemble_asm (nodes[i].u.a->asm_str);
>           break;
> diff --git a/gcc/varpool.c b/gcc/varpool.c
> index ab615fa..e5f991e 100644
> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -733,11 +733,6 @@ symbol_table::output_variables (void)
>  
>    timevar_push (TV_VAROUT);
>  
> -  FOR_EACH_VARIABLE (node)
> -    if (!node->definition
> -       && !DECL_HAS_VALUE_EXPR_P (node->decl)
> -       && !DECL_HARD_REGISTER (node->decl))
> -      assemble_undefined_decl (node->decl);
>    FOR_EACH_DEFINED_VARIABLE (node)
>      {
>        /* Handled in output_in_order.  */
> @@ -747,20 +742,19 @@ symbol_table::output_variables (void)
>        node->finalize_named_section_flags ();
>      }
>  
> -  FOR_EACH_DEFINED_VARIABLE (node)
> +  /* There is a similar loop in output_in_order.  Please keep them in sync.  */
> +  FOR_EACH_VARIABLE (node)
>      {
>        /* Handled in output_in_order.  */
>        if (node->no_reorder)
>         continue;
> -#ifdef ACCEL_COMPILER
> -      /* Do not assemble "omp declare target link" vars.  */
> -      if (DECL_HAS_VALUE_EXPR_P (node->decl)
> -         && lookup_attribute ("omp declare target link",
> -                              DECL_ATTRIBUTES (node->decl)))
> +      if (DECL_HARD_REGISTER (node->decl)
> +         || DECL_HAS_VALUE_EXPR_P (node->decl))
>         continue;
> -#endif
> -      if (node->assemble_decl ())
> -        changed = true;
> +      if (node->definition)
> +       changed |= node->assemble_decl ();
> +      else
> +       assemble_undefined_decl (node->decl);
>      }
>    timevar_pop (TV_VAROUT);
>    return changed;
> 
> 

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

On Fri, 1 Jul 2016, Alexander Monakov wrote:
> On Thu, 23 Jun 2016, Alexander Monakov wrote:
> > Hi,
> > 
> > I've discovered that this assert in my patch was too restrictive:
> > 
> > +      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
> > +	{
> > +	  gcc_checking_assert (lookup_attribute ("omp declare target link",
> > +						 DECL_ATTRIBUTES (pv->decl)));
> > 
> > Testing for the nvptx target uncovered that there's another case where a
> > global variable would have a value expr: emutls.  Sorry for not spotting it
> > earlier (but at least the new assert did its job).  I think we should always
> > skip here over decls that have value-exprs, just like hard-reg vars are
> > skipped.  The following patch does that.  Is this still OK?
> 
> Ping.

Ping^2.

> > (bootstrapped/regtested on x86-64)
> > 
> > Alexander
> > 
> > 	* cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF.
> > 	(output_in_order): Loop over undefined variables too.  Output them
> > 	via assemble_undefined_decl.  Skip variables that correspond to hard
> > 	registers or have value-exprs.
> > 	* varpool.c (symbol_table::output_variables): Handle undefined
> > 	variables together with defined ones.
> >  
> > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> > index 4bfcad7..e30fe6e 100644
> > --- a/gcc/cgraphunit.c
> > +++ b/gcc/cgraphunit.c
> > @@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind
> >    ORDER_UNDEFINED = 0,
> >    ORDER_FUNCTION,
> >    ORDER_VAR,
> > +  ORDER_VAR_UNDEF,
> >    ORDER_ASM
> >  };
> >  
> > @@ -2187,16 +2188,20 @@ output_in_order (bool no_reorder)
> >         }
> >      }
> >  
> > -  FOR_EACH_DEFINED_VARIABLE (pv)
> > -    if (!DECL_EXTERNAL (pv->decl))
> > -      {
> > -       if (no_reorder && !pv->no_reorder)
> > -           continue;
> > -       i = pv->order;
> > -       gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
> > -       nodes[i].kind = ORDER_VAR;
> > -       nodes[i].u.v = pv;
> > -      }
> > +  /* There is a similar loop in symbol_table::output_variables.
> > +     Please keep them in sync.  */
> > +  FOR_EACH_VARIABLE (pv)
> > +    {
> > +      if (no_reorder && !pv->no_reorder)
> > +       continue;
> > +      if (DECL_HARD_REGISTER (pv->decl)
> > +         || DECL_HAS_VALUE_EXPR_P (pv->decl))
> > +       continue;
> > +      i = pv->order;
> > +      gcc_assert (nodes[i].kind == ORDER_UNDEFINED);
> > +      nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF;
> > +      nodes[i].u.v = pv;
> > +    }
> >  
> >    for (pa = symtab->first_asm_symbol (); pa; pa = pa->next)
> >      {
> > @@ -2222,16 +2227,13 @@ output_in_order (bool no_reorder)
> >           break;
> >  
> >         case ORDER_VAR:
> > -#ifdef ACCEL_COMPILER
> > -         /* Do not assemble "omp declare target link" vars.  */
> > -         if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl)
> > -             && lookup_attribute ("omp declare target link",
> > -                                  DECL_ATTRIBUTES (nodes[i].u.v->decl)))
> > -           break;
> > -#endif
> >           nodes[i].u.v->assemble_decl ();
> >           break;
> >  
> > +       case ORDER_VAR_UNDEF:
> > +         assemble_undefined_decl (nodes[i].u.v->decl);
> > +         break;
> > +
> >         case ORDER_ASM:
> >           assemble_asm (nodes[i].u.a->asm_str);
> >           break;
> > diff --git a/gcc/varpool.c b/gcc/varpool.c
> > index ab615fa..e5f991e 100644
> > --- a/gcc/varpool.c
> > +++ b/gcc/varpool.c
> > @@ -733,11 +733,6 @@ symbol_table::output_variables (void)
> >  
> >    timevar_push (TV_VAROUT);
> >  
> > -  FOR_EACH_VARIABLE (node)
> > -    if (!node->definition
> > -       && !DECL_HAS_VALUE_EXPR_P (node->decl)
> > -       && !DECL_HARD_REGISTER (node->decl))
> > -      assemble_undefined_decl (node->decl);
> >    FOR_EACH_DEFINED_VARIABLE (node)
> >      {
> >        /* Handled in output_in_order.  */
> > @@ -747,20 +742,19 @@ symbol_table::output_variables (void)
> >        node->finalize_named_section_flags ();
> >      }
> >  
> > -  FOR_EACH_DEFINED_VARIABLE (node)
> > +  /* There is a similar loop in output_in_order.  Please keep them in sync.  */
> > +  FOR_EACH_VARIABLE (node)
> >      {
> >        /* Handled in output_in_order.  */
> >        if (node->no_reorder)
> >         continue;
> > -#ifdef ACCEL_COMPILER
> > -      /* Do not assemble "omp declare target link" vars.  */
> > -      if (DECL_HAS_VALUE_EXPR_P (node->decl)
> > -         && lookup_attribute ("omp declare target link",
> > -                              DECL_ATTRIBUTES (node->decl)))
> > +      if (DECL_HARD_REGISTER (node->decl)
> > +         || DECL_HAS_VALUE_EXPR_P (node->decl))
> >         continue;
> > -#endif
> > -      if (node->assemble_decl ())
> > -        changed = true;
> > +      if (node->definition)
> > +       changed |= node->assemble_decl ();
> > +      else
> > +       assemble_undefined_decl (node->decl);
> >      }
> >    timevar_pop (TV_VAROUT);
> >    return changed;
> > 
> > 

Re: [PATCH] Handle undefined extern vars in output_in_order

[Jeff Law]

On 06/23/2016 08:45 AM, Alexander Monakov wrote:
> Hi,
>
> I've discovered that this assert in my patch was too restrictive:
>
> +      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
> +	{
> +	  gcc_checking_assert (lookup_attribute ("omp declare target link",
> +						 DECL_ATTRIBUTES (pv->decl)));
>
> Testing for the nvptx target uncovered that there's another case where a
> global variable would have a value expr: emutls.  Sorry for not spotting it
> earlier (but at least the new assert did its job).  I think we should always
> skip here over decls that have value-exprs, just like hard-reg vars are
> skipped.  The following patch does that.  Is this still OK?
>
> (bootstrapped/regtested on x86-64)
>
> Alexander
>
> 	* cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF.
> 	(output_in_order): Loop over undefined variables too.  Output them
> 	via assemble_undefined_decl.  Skip variables that correspond to hard
> 	registers or have value-exprs.
> 	* varpool.c (symbol_table::output_variables): Handle undefined
> 	variables together with defined ones.
I haven't followed this issue at all.  So bear with me if I ask a 
question you've already answered.

Is the point here to be able to deduce what symbols are external & 
undefined and emit some kind of directive to the assembler in that case? 
  Avoiding duplicates as well as symbols which may have had originally 
looked like external & undefined, but later we find a definition?

The reason I ask someone might be able to simplify a bit of the PA 
backend if that's the goal here.  The PA (when using the SOM object 
format) has similar needs.  We solved it by queuing up everything that 
might need "importing".  Then at the end of the compilation unit, we'd 
walk that queue of symbols emitting the proper magic.

Jeff

Re: [PATCH] Handle undefined extern vars in output_in_order

[Alexander Monakov]

On Thu, 14 Jul 2016, Jeff Law wrote:
> Is the point here to be able to deduce what symbols are external & undefined
> and emit some kind of directive to the assembler in that case?

Yes, PTX assembly requires that properly typed declarations are emitted for
external references.  Today, the NVPTX backend in GCC performs that internally
for function symbols, and relies on the middle-end to do that for data symbols,
but when the port was added that was implemented only when -ftoplevel-reorder is
in effect (the default).

The point of my patch is to handle it under -fno-toplevel-reorder in the same
way.

> Avoiding duplicates

this is not required for PTX, but nevertheless a good cleanup we get for free

> as well as symbols which may have had originally looked like external &
> undefined, but later we find a definition?

I think this only would be a problem if GCC continued to support
-fno-unit-at-a-time.  AFAIK today GCC's symtab reflects symbol availability
exactly, because the whole translation unit has been read upfront.

> The reason I ask someone might be able to simplify a bit of the PA backend if
> that's the goal here.  The PA (when using the SOM object format) has similar
> needs.  We solved it by queuing up everything that might need "importing".
> Then at the end of the compilation unit, we'd walk that queue of symbols
> emitting the proper magic.

*nod*, although as I mentioned above today it's handled in a generic way only
for undefined data symbols, not undefined function symbols.

Alexander

Re: [PATCH] Handle undefined extern vars in output_in_order

[Jeff Law]

On 07/14/2016 12:33 PM, Alexander Monakov wrote:
> On Thu, 14 Jul 2016, Jeff Law wrote:
>> Is the point here to be able to deduce what symbols are external & undefined
>> and emit some kind of directive to the assembler in that case?
>
> Yes, PTX assembly requires that properly typed declarations are emitted for
> external references.  Today, the NVPTX backend in GCC performs that internally
> for function symbols, and relies on the middle-end to do that for data symbols,
> but when the port was added that was implemented only when -ftoplevel-reorder is
> in effect (the default).
Right.  I remember noting that in my brief review of the PTX assembly 
specs and figuring it wouldn't be a big deal since we had a functional 
hack in place for the PA/SOM which has similar requirements.

>
> *nod*, although as I mentioned above today it's handled in a generic way only
> for undefined data symbols, not undefined function symbols.
Understood.  Quickly reviewing the PA code, the other bit of magic is we 
avoided importing a symbol which was declared as extern, but never 
actually used.  IIRC that caused the HP/SOM linker to core dump. 
Hopefully the PTX assembler/linker/finalizer handles that case more 
gracefully...

Jeff

Re: [PATCH] Handle undefined extern vars in output_in_order

[Jeff Law]

On 06/23/2016 08:45 AM, Alexander Monakov wrote:
> Hi,
>
> I've discovered that this assert in my patch was too restrictive:
>
> +      if (DECL_HAS_VALUE_EXPR_P (pv->decl))
> +	{
> +	  gcc_checking_assert (lookup_attribute ("omp declare target link",
> +						 DECL_ATTRIBUTES (pv->decl)));
>
> Testing for the nvptx target uncovered that there's another case where a
> global variable would have a value expr: emutls.  Sorry for not spotting it
> earlier (but at least the new assert did its job).  I think we should always
> skip here over decls that have value-exprs, just like hard-reg vars are
> skipped.  The following patch does that.  Is this still OK?
>
> (bootstrapped/regtested on x86-64)
>
> Alexander
>
> 	* cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF.
> 	(output_in_order): Loop over undefined variables too.  Output them
> 	via assemble_undefined_decl.  Skip variables that correspond to hard
> 	registers or have value-exprs.
> 	* varpool.c (symbol_table::output_variables): Handle undefined
> 	variables together with defined ones.
OK.  Thanks for your patience.
jeff