varpool/constpool bug

varpool/constpool bug

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

A recent PTX patch of mine exposed a latent bug in varpool handling by exposing 
more CSE opportunities.

In the attached testcase:

char *p;

void foo ()
{
   p = "abc\n";
   while (*p != '\n')
     p++;
}

the RTL CSE pass checks whether SYMBOL_REF (p) and SYMBOL_REF ($LCO) are 
related.  LC0 is the constant pool entry for the string constant.  In doing 
this, we end up in compare_base_decls (alias.c), and decl_in_symtab_p thinks 
'$LC0' should be in the symtab (it's a TREE_STATIC VAR_DECL).  Consequently we 
continue on to  symtab_node::get_create (base2), and create a symtab entry for 
$LCO.  That entry thinks $LCO is defined in another TU.  (this is the point at 
which PTX blows up because $LCO is emitted to the assembly file as both a static 
  definition and an extern declaration.)

Either build_constant_desc (varasm.c) should put constants into the varpool or 
decl_in_symtab_p should ignore constant pool VAR_DECLS.  It appears to me that 
the latter is the right choice, as constant pool entries can't be related to 
regular varpool entries (right?)

Thus this patch augments the decl_in_symtab_p function to ignore constant pool 
entries.  While debugging I found the logic in compare_base_decls of 'in_symtab1 
!= in_symtab2 || !in_symtab1' to be rather confusing, and simplified it to be 
(the moral equivalent of) '!in_symtab1 || !in_symtab2'.

This looks target-agnostic, and manifests on (at least) x86_64-linux too -- 
except most targets don't need to emit declarations of undefined externs, so 
they don't explode.  Without the patch, the attached testcase shows an 
additional varpool entry of:
*.LC0/2 (*.LC0) @0x7fa5c7d3a400
   Type: variable
   Visibility: asm_written artificial
   References:
   Referring:
   Availability: not_available
   Varpool flags: initialized read-only const-value-known


ok?

nathan

[-- Attachment #2: trunk-varpool.patch --]
[-- Type: text/x-patch, Size: 1827 bytes --]

2015-12-19  Nathan Sidwell  <nathan@acm.org>

	gcc/
	* alias.c (compare_base_decls): Simplify in-symtab check.
	* cgraph.h (decl_in_symtab_p): Check DECL_IN_CONSTANT_POOL.

	gcc/testsuite/
	* gcc.dg/alias-15.c: New. 

Index: alias.c
===================================================================
--- alias.c	(revision 231841)
+++ alias.c	(working copy)
@@ -2038,13 +2038,12 @@ compare_base_decls (tree base1, tree bas
   if (base1 == base2)
     return 1;
 
-  bool in_symtab1 = decl_in_symtab_p (base1);
-  bool in_symtab2 = decl_in_symtab_p (base2);
-
   /* Declarations of non-automatic variables may have aliases.  All other
      decls are unique.  */
-  if (in_symtab1 != in_symtab2 || !in_symtab1)
+  if (!decl_in_symtab_p (base1)
+      || !decl_in_symtab_p (base2))
     return 0;
+
   ret = symtab_node::get_create (base1)->equal_address_to
 		 (symtab_node::get_create (base2), true);
   if (ret == 2)
Index: cgraph.h
===================================================================
--- cgraph.h	(revision 231841)
+++ cgraph.h	(working copy)
@@ -2301,6 +2301,7 @@ decl_in_symtab_p (const_tree decl)
 {
   return (TREE_CODE (decl) == FUNCTION_DECL
           || (TREE_CODE (decl) == VAR_DECL
+	      && !DECL_IN_CONSTANT_POOL (decl)
 	      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))));
 }
 
Index: testsuite/gcc.dg/alias-15.c
===================================================================
--- testsuite/gcc.dg/alias-15.c	(revision 0)
+++ testsuite/gcc.dg/alias-15.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-additional-options  "-O2 -fdump-ipa-cgraph" } */
+
+/* RTL-level CSE shouldn't introduce LCO (for the string) into varpool */
+char *p;
+
+void foo ()
+{
+  p = "abc\n";
+
+  while (*p != '\n')
+    p++;
+}
+
+/* { dg-final { scan-ipa-dump-not "LC0" "cgraph" } } */

Re: varpool/constpool bug

[Jeff Law]

On 12/19/2015 09:47 AM, Nathan Sidwell wrote:
>
> Either build_constant_desc (varasm.c) should put constants into the
> varpool or decl_in_symtab_p should ignore constant pool VAR_DECLS.  It
> appears to me that the latter is the right choice, as constant pool
> entries can't be related to regular varpool entries (right?)
>
> Thus this patch augments the decl_in_symtab_p function to ignore
> constant pool entries.  While debugging I found the logic in
> compare_base_decls of 'in_symtab1 != in_symtab2 || !in_symtab1' to be
> rather confusing, and simplified it to be (the moral equivalent of)
> '!in_symtab1 || !in_symtab2'.
>
> This looks target-agnostic, and manifests on (at least) x86_64-linux too
> -- except most targets don't need to emit declarations of undefined
> externs, so they don't explode.
The only other target I'm aware of where this is an issue is the PA. 
And it may only be an issue in the older 32-bit SOM runtime. 
Essentially you have to "import" everything you use from other TUs and 
bad things happen if you "import" something that is actually file scoped 
in the current TU.


>
>
> ok?
With some kind of comment in decl_in_symtab_p indicating why we need to 
check and filter on !DECL_IN_CONSTANT_POOL this is OK.


jeff

Re: varpool/constpool bug

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

On 12/21/15 14:45, Jeff Law wrote:

> With some kind of comment in decl_in_symtab_p indicating why we need to check
> and filter on !DECL_IN_CONSTANT_POOL this is OK.

Done, thanks.  (I half guessed HPPA might be such a port :)

nathan


[-- Attachment #2: trunk-varpool.patch --]
[-- Type: text/x-patch, Size: 2395 bytes --]

2015-12-23  Nathan Sidwell  <nathan@acm.org>

	gcc/
	* alias.c (compare_base_decls): Simplify in-symtab check.
	* cgraph.h (decl_in_symtab_p): Check DECL_IN_CONSTANT_POOL.

	testsuite/
	* gcc.dg/alias-15.c: New. 

Index: alias.c
===================================================================
--- alias.c	(revision 231927)
+++ alias.c	(working copy)
@@ -2038,13 +2038,12 @@ compare_base_decls (tree base1, tree bas
   if (base1 == base2)
     return 1;
 
-  bool in_symtab1 = decl_in_symtab_p (base1);
-  bool in_symtab2 = decl_in_symtab_p (base2);
-
   /* Declarations of non-automatic variables may have aliases.  All other
      decls are unique.  */
-  if (in_symtab1 != in_symtab2 || !in_symtab1)
+  if (!decl_in_symtab_p (base1)
+      || !decl_in_symtab_p (base2))
     return 0;
+
   ret = symtab_node::get_create (base1)->equal_address_to
 		 (symtab_node::get_create (base2), true);
   if (ret == 2)
Index: cgraph.h
===================================================================
--- cgraph.h	(revision 231927)
+++ cgraph.h	(working copy)
@@ -2294,13 +2294,19 @@ symtab_node::real_symbol_p (void)
 }
 
 /* Return true if DECL should have entry in symbol table if used.
-   Those are functions and static & external veriables*/
+   Those are functions and static & external non-constpool variables.
+   We do not expect constant pool variables in the varpool, as they're
+   not related to other variables, and simply lazily inserting them
+   using the regular interface results in varpool thinking they are
+   externally provided -- which results in erroneous assembly emission
+   as an undefined decl.  */
 
 static inline bool
 decl_in_symtab_p (const_tree decl)
 {
   return (TREE_CODE (decl) == FUNCTION_DECL
           || (TREE_CODE (decl) == VAR_DECL
+	      && !DECL_IN_CONSTANT_POOL (decl)
 	      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))));
 }
 
Index: testsuite/gcc.dg/alias-15.c
===================================================================
--- testsuite/gcc.dg/alias-15.c	(revision 0)
+++ testsuite/gcc.dg/alias-15.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-additional-options  "-O2 -fdump-ipa-cgraph" } */
+
+/* RTL-level CSE shouldn't introduce LCO (for the string) into varpool */
+char *p;
+
+void foo ()
+{
+  p = "abc\n";
+
+  while (*p != '\n')
+    p++;
+}
+
+/* { dg-final { scan-ipa-dump-not "LC0" "cgraph" } } */

Re: varpool/constpool bug

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On 12/23/15 09:48, Nathan Sidwell wrote:
> On 12/21/15 14:45, Jeff Law wrote:
>
>> With some kind of comment in decl_in_symtab_p indicating why we need to check
>> and filter on !DECL_IN_CONSTANT_POOL this is OK.
>
> Done, thanks.  (I half guessed HPPA might be such a port :)

sigh, there are  other routes by which constpool entries get into the varpool, 
and it appears other checks using symtab_in_varpool_p to figure out whether they 
need to do something.  Leading to powerpc having a problem. 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69033

powerpc inserts an array initializer constant pool entry into the varpool via 
tree_output_constant_def (varasm.c), that has an explicit call to:
  varpool_node::finalize_decl (decl);

so it seems intentional  that constpool objects can end up also in the varpool. 
  I notice that via this route varpool thinks the constant's value is available, 
  which is different to the behaviour when insertionwas via alias checking.

So, I think that either
1) alias checking shouldn't create varpool entries (i.e. use varpool::get rather 
than varpool::get_create). It wouldn't need to use the symtab_in_varpool_p 
predicate in that case.
2) Or varpool::get_create should take note of DECL_IN_CONSTANT_POOL and declare 
the value available
3) or build_constant_desc (varasm.c) should explicitly put the constant in the 
varpool, to match tree_output_constant_def?

I have reverted the cgraph.h change and am experimenting with option #1

nathan

[-- Attachment #2: trunk-varpool-revert.patch --]
[-- Type: text/x-patch, Size: 1654 bytes --]

2015-12-23  Nathan Sidwell  <nathan@acm.org>

	gcc/
	* cgraph.h (decl_in_symtab_p): Revert check DECL_IN_CONSTANT_POOL.

	gcc/testsuite/
	* gcc.dg/alias-15.c: Revert.

Index: cgraph.h
===================================================================
--- cgraph.h	(revision 231929)
+++ cgraph.h	(working copy)
@@ -2294,19 +2294,13 @@ symtab_node::real_symbol_p (void)
 }
 
 /* Return true if DECL should have entry in symbol table if used.
-   Those are functions and static & external non-constpool variables.
-   We do not expect constant pool variables in the varpool, as they're
-   not related to other variables, and simply lazily inserting them
-   using the regular interface results in varpool thinking they are
-   externally provided -- which results in erroneous assembly emission
-   as an undefined decl.  */
+   Those are functions and static & external veriables*/
 
 static inline bool
 decl_in_symtab_p (const_tree decl)
 {
   return (TREE_CODE (decl) == FUNCTION_DECL
           || (TREE_CODE (decl) == VAR_DECL
-	      && !DECL_IN_CONSTANT_POOL (decl)
 	      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))));
 }
 
Index: testsuite/gcc.dg/alias-15.c
===================================================================
--- testsuite/gcc.dg/alias-15.c	(revision 231929)
+++ testsuite/gcc.dg/alias-15.c	(working copy)
@@ -1,15 +0,0 @@
-/* { dg-do compile } */
-/* { dg-additional-options  "-O2 -fdump-ipa-cgraph" } */
-
-/* RTL-level CSE shouldn't introduce LCO (for the string) into varpool */
-char *p;
-
-void foo ()
-{
-  p = "abc\n";
-
-  while (*p != '\n')
-    p++;
-}
-
-/* { dg-final { scan-ipa-dump-not "LC0" "cgraph" } } */

Re: varpool/constpool bug

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

My patch to stop constant pool objects accidentally ending up in the varpool 
caused problems with (at least) powerpc. 
(https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02100.html) Hence reverted.

This patch changes compare_base_decls to simply use the varpool getter, rather 
than get_create.  We still need the preceding decl_in_symtab_p to filter out 
decls that should never be in the varpool (the getter has an assert to check 
you're not trying to abuse it).

ok?

nathan

[-- Attachment #2: trunk-alias.patch --]
[-- Type: text/x-patch, Size: 1355 bytes --]

2016-01-04  Nathan Sidwell  <nathan@acm.org>

	gcc/
	* alias.c (compare_base_decls): Use symtab_node::get.

	gcc/testsuite/
	* gcc.dg/alias-15.c: New.

Index: alias.c
===================================================================
--- alias.c	(revision 232057)
+++ alias.c	(working copy)
@@ -2044,8 +2044,15 @@ compare_base_decls (tree base1, tree bas
       || !decl_in_symtab_p (base2))
     return 0;
 
-  ret = symtab_node::get_create (base1)->equal_address_to
-		 (symtab_node::get_create (base2), true);
+  /* Don't cause symbols to be inserted by the act of checking.  */
+  symtab_node *node1 = symtab_node::get (base1);
+  if (!node1)
+    return 0;
+  symtab_node *node2 = symtab_node::get (base2);
+  if (!node2)
+    return 0;
+  
+  ret = node1->equal_address_to (node2, true);
   if (ret == 2)
     return -1;
   return ret;
Index: testsuite/gcc.dg/alias-15.c
===================================================================
--- testsuite/gcc.dg/alias-15.c	(revision 0)
+++ testsuite/gcc.dg/alias-15.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-additional-options  "-O2 -fdump-ipa-cgraph" } */
+
+/* RTL-level CSE shouldn't introduce LCO (for the string) into varpool */
+char *p;
+
+void foo ()
+{
+  p = "abc\n";
+
+  while (*p != '\n')
+    p++;
+}
+
+/* { dg-final { scan-ipa-dump-not "LC0" "cgraph" } } */

Re: varpool/constpool bug

[Jeff Law]

On 01/04/2016 08:57 AM, Nathan Sidwell wrote:
> My patch to stop constant pool objects accidentally ending up in the
> varpool caused problems with (at least) powerpc.
> (https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02100.html) Hence reverted.
>
> This patch changes compare_base_decls to simply use the varpool getter,
> rather than get_create.  We still need the preceding decl_in_symtab_p to
> filter out decls that should never be in the varpool (the getter has an
> assert to check you're not trying to abuse it).
>
> ok?
Once it passes the usual bootstrap & regression testing.

Looking at it again, it seems "obvious" now that the act of comparing 
things for alias analysis shouldn't be inserting new things into the tables.


jeff

Re: varpool/constpool bug

[Richard Biener]

On Mon, Jan 4, 2016 at 7:53 PM, Jeff Law <law@redhat.com> wrote:
> On 01/04/2016 08:57 AM, Nathan Sidwell wrote:
>>
>> My patch to stop constant pool objects accidentally ending up in the
>> varpool caused problems with (at least) powerpc.
>> (https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02100.html) Hence reverted.
>>
>> This patch changes compare_base_decls to simply use the varpool getter,
>> rather than get_create.  We still need the preceding decl_in_symtab_p to
>> filter out decls that should never be in the varpool (the getter has an
>> assert to check you're not trying to abuse it).
>>
>> ok?
>
> Once it passes the usual bootstrap & regression testing.
>
> Looking at it again, it seems "obvious" now that the act of comparing things
> for alias analysis shouldn't be inserting new things into the tables.

I asked hoza to do this change as well but he replied with a rather lengthy
answer that using get_create is correct.

Oh, and I _do_ belive that ultimatively we should have all constant pool
entries (and CONST_DECLs) in the varpool.

So, Honza, can you please chime in here and comment on the (already
committed) patches?

Richard.

>
> jeff

Re: varpool/constpool bug

[Nathan Sidwell]

On 01/08/16 05:39, Richard Biener wrote:

> Oh, and I _do_ belive that ultimatively we should have all constant pool
> entries (and CONST_DECLs) in the varpool.

FWIW that certainly seems like the right goal.

nathan

Re: varpool/constpool bug

[Jan Hubicka]

> On 01/08/16 05:39, Richard Biener wrote:
> 
> >Oh, and I _do_ belive that ultimatively we should have all constant pool
> >entries (and CONST_DECLs) in the varpool.
> 
> FWIW that certainly seems like the right goal.
Indeed it is on my TODO. I need to make CONST_DECLs to have visibility (so they can be
properly LTO partitioned).

VAR_DECL with local label name should work with symtab.  I noticed the thread
and plan to look more into the issue, just did not have chance to do it yet
(hopefully during weekend)

Honza
> 
> nathan

Re: varpool/constpool bug

[Richard Biener]

On January 8, 2016 4:13:17 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On 01/08/16 05:39, Richard Biener wrote:
>> 
>> >Oh, and I _do_ belive that ultimatively we should have all constant
>pool
>> >entries (and CONST_DECLs) in the varpool.
>> 
>> FWIW that certainly seems like the right goal.
>Indeed it is on my TODO. I need to make CONST_DECLs to have visibility
>(so they can be
>properly LTO partitioned).

There is also the wrong-code bug regarding to STRING_CST compares.
I'd like to disallow &STRING_CST in the IL and instead force a CONST_DECL for those.

Richard.

>VAR_DECL with local label name should work with symtab.  I noticed the
>thread
>and plan to look more into the issue, just did not have chance to do it
>yet
>(hopefully during weekend)
>
>Honza
>> 
>> nathan