| Message ID | 568A967F.4060203@acm.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
> 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
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
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" } } */