* varpool/constpool bug
@ 2015-12-19 16:47 Nathan Sidwell
2015-12-21 19:45 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Sidwell @ 2015-12-19 16:47 UTC (permalink / raw)
To: GCC Patches
[-- 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" } } */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2015-12-19 16:47 varpool/constpool bug Nathan Sidwell
@ 2015-12-21 19:45 ` Jeff Law
2015-12-23 14:48 ` Nathan Sidwell
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2015-12-21 19:45 UTC (permalink / raw)
To: Nathan Sidwell, GCC Patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2015-12-21 19:45 ` Jeff Law
@ 2015-12-23 14:48 ` Nathan Sidwell
2015-12-23 18:25 ` Nathan Sidwell
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Sidwell @ 2015-12-23 14:48 UTC (permalink / raw)
To: Jeff Law, GCC Patches
[-- 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" } } */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2015-12-23 14:48 ` Nathan Sidwell
@ 2015-12-23 18:25 ` Nathan Sidwell
2016-01-04 15:57 ` Nathan Sidwell
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Sidwell @ 2015-12-23 18:25 UTC (permalink / raw)
To: Jeff Law, GCC Patches; +Cc: seurer
[-- 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" } } */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2015-12-23 18:25 ` Nathan Sidwell
@ 2016-01-04 15:57 ` Nathan Sidwell
2016-01-04 18:53 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Sidwell @ 2016-01-04 15:57 UTC (permalink / raw)
To: Jeff Law, GCC Patches; +Cc: seurer
[-- 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" } } */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2016-01-04 15:57 ` Nathan Sidwell
@ 2016-01-04 18:53 ` Jeff Law
2016-01-08 10:39 ` Richard Biener
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2016-01-04 18:53 UTC (permalink / raw)
To: Nathan Sidwell, GCC Patches; +Cc: seurer
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2016-01-04 18:53 ` Jeff Law
@ 2016-01-08 10:39 ` Richard Biener
2016-01-08 14:04 ` Nathan Sidwell
0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2016-01-08 10:39 UTC (permalink / raw)
To: Jeff Law, Jan Hubicka; +Cc: Nathan Sidwell, GCC Patches, seurer
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2016-01-08 10:39 ` Richard Biener
@ 2016-01-08 14:04 ` Nathan Sidwell
2016-01-08 15:13 ` Jan Hubicka
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Sidwell @ 2016-01-08 14:04 UTC (permalink / raw)
To: Richard Biener, Jeff Law, Jan Hubicka; +Cc: GCC Patches, seurer
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2016-01-08 14:04 ` Nathan Sidwell
@ 2016-01-08 15:13 ` Jan Hubicka
2016-01-08 15:42 ` Richard Biener
0 siblings, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2016-01-08 15:13 UTC (permalink / raw)
To: Nathan Sidwell; +Cc: Richard Biener, Jeff Law, Jan Hubicka, GCC Patches, seurer
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: varpool/constpool bug
2016-01-08 15:13 ` Jan Hubicka
@ 2016-01-08 15:42 ` Richard Biener
0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2016-01-08 15:42 UTC (permalink / raw)
To: Jan Hubicka, Nathan Sidwell; +Cc: Jeff Law, GCC Patches, seurer
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-01-08 15:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-19 16:47 varpool/constpool bug Nathan Sidwell
2015-12-21 19:45 ` Jeff Law
2015-12-23 14:48 ` Nathan Sidwell
2015-12-23 18:25 ` Nathan Sidwell
2016-01-04 15:57 ` Nathan Sidwell
2016-01-04 18:53 ` Jeff Law
2016-01-08 10:39 ` Richard Biener
2016-01-08 14:04 ` Nathan Sidwell
2016-01-08 15:13 ` Jan Hubicka
2016-01-08 15:42 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).