| Message ID | 20140404061826.GC16291@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Fri, 4 Apr 2014, Jan Hubicka wrote: > Hi, > here is an updated version of my earlier ipa.c change. It turns out that the > problem was that I did not drop always_inline. > In this version I just drop always_inline attribute on all functions whose body > is removed. The patch will affect non-LTO compilation, too, but IMO only by > making us to not inline&diagnose the cases where indirect call to always inline > is turned direct in between early opts and inline. Does that seem acceptable? > (I personally would preffer this over inventing yet another way to special case > always_inline for LTO only; we never made any strong promises on always_inline > and indirect calls) I think it's fine if indirect calls to always-inline fns go to an offline copy (or cause a link-time error if there is no offline copy). I've always thought that taking the address of an always_inline fn should get you the address of an "external" symbol (an inline "clone" isn't addressable). So the patch is fine with me. Thanks, Richard. > Honza > > * lto-cgraph.c (input_overwrite_node): Check that partitioning > flags are set only during streaming. > * ipa.c (process_references, walk_polymorphic_call_targets, > symtab_remove_unreachable_nodes): Drop bodies of always inline > after early inlining. > (symtab_remove_unreachable_nodes): Remove always_inline attribute. > > * gcc.dg/lto/pr59626_0.c: New testcase. > * gcc.dg/lto/pr59626_1.c: New testcase. > Index: lto-cgraph.c > =================================================================== > --- lto-cgraph.c (revision 209062) > +++ lto-cgraph.c (working copy) > @@ -1001,6 +1001,9 @@ input_overwrite_node (struct lto_file_de > node->thunk.thunk_p = bp_unpack_value (bp, 1); > node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution, > LDPR_NUM_KNOWN); > + gcc_assert (flag_ltrans > + || (!node->in_other_partition > + && !node->used_from_other_partition)); > } > > /* Return string alias is alias of. */ > @@ -1169,6 +1172,9 @@ input_varpool_node (struct lto_file_decl > node->same_comdat_group = (symtab_node *) (intptr_t) ref; > node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution, > LDPR_NUM_KNOWN); > + gcc_assert (flag_ltrans > + || (!node->in_other_partition > + && !node->used_from_other_partition)); > > return node; > } > Index: ipa.c > =================================================================== > --- ipa.c (revision 209062) > +++ ipa.c (working copy) > @@ -139,7 +139,10 @@ process_references (struct ipa_ref_list > > if (node->definition && !node->in_other_partition > && ((!DECL_EXTERNAL (node->decl) || node->alias) > - || (before_inlining_p > + || (((before_inlining_p > + && (cgraph_state < CGRAPH_STATE_IPA_SSA > + || !lookup_attribute ("always_inline", > + DECL_ATTRIBUTES (node->decl))))) > /* We use variable constructors during late complation for > constant folding. Keep references alive so partitioning > knows about potential references. */ > @@ -191,7 +194,10 @@ walk_polymorphic_call_targets (pointer_s > /* Prior inlining, keep alive bodies of possible targets for > devirtualization. */ > if (n->definition > - && before_inlining_p) > + && (before_inlining_p > + && (cgraph_state < CGRAPH_STATE_IPA_SSA > + || !lookup_attribute ("always_inline", > + DECL_ATTRIBUTES (n->decl))))) > pointer_set_insert (reachable, n); > > /* Even after inlining we want to keep the possible targets in the > @@ -491,6 +497,12 @@ symtab_remove_unreachable_nodes (bool be > node->alias = false; > node->thunk.thunk_p = false; > node->weakref = false; > + /* After early inlining we drop always_inline attributes on > + bodies of functions that are still referenced (have their > + address taken). */ > + DECL_ATTRIBUTES (node->decl) > + = remove_attribute ("always_inline", > + DECL_ATTRIBUTES (node->decl)); > if (!node->in_other_partition) > node->local.local = false; > cgraph_node_remove_callees (node); > Index: testsuite/gcc.dg/lto/pr59626_1.c > =================================================================== > --- testsuite/gcc.dg/lto/pr59626_1.c (revision 0) > +++ testsuite/gcc.dg/lto/pr59626_1.c (revision 0) > @@ -0,0 +1,4 @@ > +int bar (int (*fn)(const char *)) > +{ > + return fn ("0"); > +} > Index: testsuite/gcc.dg/lto/pr59626_0.c > =================================================================== > --- testsuite/gcc.dg/lto/pr59626_0.c (revision 0) > +++ testsuite/gcc.dg/lto/pr59626_0.c (revision 0) > @@ -0,0 +1,15 @@ > +/* { dg-lto-do run } */ > + > +int __atoi (const char *) __asm__("atoi"); > +extern inline __attribute__((always_inline,gnu_inline)) > +int atoi (const char *x) > +{ > + return __atoi (x); > +} > + > +int bar (int (*)(const char *)); > + > +int main() > +{ > + return bar (atoi); > +} > >
> On Fri, 4 Apr 2014, Jan Hubicka wrote: > > > Hi, > > here is an updated version of my earlier ipa.c change. It turns out that the > > problem was that I did not drop always_inline. > > In this version I just drop always_inline attribute on all functions whose body > > is removed. The patch will affect non-LTO compilation, too, but IMO only by > > making us to not inline&diagnose the cases where indirect call to always inline > > is turned direct in between early opts and inline. Does that seem acceptable? > > (I personally would preffer this over inventing yet another way to special case > > always_inline for LTO only; we never made any strong promises on always_inline > > and indirect calls) > > I think it's fine if indirect calls to always-inline fns go to an > offline copy (or cause a link-time error if there is no offline copy). > I've always thought that taking the address of an always_inline fn > should get you the address of an "external" symbol (an inline "clone" > isn't addressable). > > So the patch is fine with me. OK, I comitted it. I forgot to mention that with fortified bootstrap I got quite few extra warning on return value of some functions being ignored (such as fscanf). Are those expected to appear only with fortification and are we expected to cowardly ignore them? Honza > > Thanks, > Richard. > > > Honza > > > > * lto-cgraph.c (input_overwrite_node): Check that partitioning > > flags are set only during streaming. > > * ipa.c (process_references, walk_polymorphic_call_targets, > > symtab_remove_unreachable_nodes): Drop bodies of always inline > > after early inlining. > > (symtab_remove_unreachable_nodes): Remove always_inline attribute. > > > > * gcc.dg/lto/pr59626_0.c: New testcase. > > * gcc.dg/lto/pr59626_1.c: New testcase. > > Index: lto-cgraph.c > > =================================================================== > > --- lto-cgraph.c (revision 209062) > > +++ lto-cgraph.c (working copy) > > @@ -1001,6 +1001,9 @@ input_overwrite_node (struct lto_file_de > > node->thunk.thunk_p = bp_unpack_value (bp, 1); > > node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution, > > LDPR_NUM_KNOWN); > > + gcc_assert (flag_ltrans > > + || (!node->in_other_partition > > + && !node->used_from_other_partition)); > > } > > > > /* Return string alias is alias of. */ > > @@ -1169,6 +1172,9 @@ input_varpool_node (struct lto_file_decl > > node->same_comdat_group = (symtab_node *) (intptr_t) ref; > > node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution, > > LDPR_NUM_KNOWN); > > + gcc_assert (flag_ltrans > > + || (!node->in_other_partition > > + && !node->used_from_other_partition)); > > > > return node; > > } > > Index: ipa.c > > =================================================================== > > --- ipa.c (revision 209062) > > +++ ipa.c (working copy) > > @@ -139,7 +139,10 @@ process_references (struct ipa_ref_list > > > > if (node->definition && !node->in_other_partition > > && ((!DECL_EXTERNAL (node->decl) || node->alias) > > - || (before_inlining_p > > + || (((before_inlining_p > > + && (cgraph_state < CGRAPH_STATE_IPA_SSA > > + || !lookup_attribute ("always_inline", > > + DECL_ATTRIBUTES (node->decl))))) > > /* We use variable constructors during late complation for > > constant folding. Keep references alive so partitioning > > knows about potential references. */ > > @@ -191,7 +194,10 @@ walk_polymorphic_call_targets (pointer_s > > /* Prior inlining, keep alive bodies of possible targets for > > devirtualization. */ > > if (n->definition > > - && before_inlining_p) > > + && (before_inlining_p > > + && (cgraph_state < CGRAPH_STATE_IPA_SSA > > + || !lookup_attribute ("always_inline", > > + DECL_ATTRIBUTES (n->decl))))) > > pointer_set_insert (reachable, n); > > > > /* Even after inlining we want to keep the possible targets in the > > @@ -491,6 +497,12 @@ symtab_remove_unreachable_nodes (bool be > > node->alias = false; > > node->thunk.thunk_p = false; > > node->weakref = false; > > + /* After early inlining we drop always_inline attributes on > > + bodies of functions that are still referenced (have their > > + address taken). */ > > + DECL_ATTRIBUTES (node->decl) > > + = remove_attribute ("always_inline", > > + DECL_ATTRIBUTES (node->decl)); > > if (!node->in_other_partition) > > node->local.local = false; > > cgraph_node_remove_callees (node); > > Index: testsuite/gcc.dg/lto/pr59626_1.c > > =================================================================== > > --- testsuite/gcc.dg/lto/pr59626_1.c (revision 0) > > +++ testsuite/gcc.dg/lto/pr59626_1.c (revision 0) > > @@ -0,0 +1,4 @@ > > +int bar (int (*fn)(const char *)) > > +{ > > + return fn ("0"); > > +} > > Index: testsuite/gcc.dg/lto/pr59626_0.c > > =================================================================== > > --- testsuite/gcc.dg/lto/pr59626_0.c (revision 0) > > +++ testsuite/gcc.dg/lto/pr59626_0.c (revision 0) > > @@ -0,0 +1,15 @@ > > +/* { dg-lto-do run } */ > > + > > +int __atoi (const char *) __asm__("atoi"); > > +extern inline __attribute__((always_inline,gnu_inline)) > > +int atoi (const char *x) > > +{ > > + return __atoi (x); > > +} > > + > > +int bar (int (*)(const char *)); > > + > > +int main() > > +{ > > + return bar (atoi); > > +} > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE / SUSE Labs > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
On Fri, 4 Apr 2014, Jan Hubicka wrote: > > On Fri, 4 Apr 2014, Jan Hubicka wrote: > > > > > Hi, > > > here is an updated version of my earlier ipa.c change. It turns out that the > > > problem was that I did not drop always_inline. > > > In this version I just drop always_inline attribute on all functions whose body > > > is removed. The patch will affect non-LTO compilation, too, but IMO only by > > > making us to not inline&diagnose the cases where indirect call to always inline > > > is turned direct in between early opts and inline. Does that seem acceptable? > > > (I personally would preffer this over inventing yet another way to special case > > > always_inline for LTO only; we never made any strong promises on always_inline > > > and indirect calls) > > > > I think it's fine if indirect calls to always-inline fns go to an > > offline copy (or cause a link-time error if there is no offline copy). > > I've always thought that taking the address of an always_inline fn > > should get you the address of an "external" symbol (an inline "clone" > > isn't addressable). > > > > So the patch is fine with me. > > OK, I comitted it. I forgot to mention that with fortified bootstrap I got > quite few extra warning on return value of some functions being ignored > (such as fscanf). Are those expected to appear only with fortification and > are we expected to cowardly ignore them? Yes, they are only expected to appear with fortification and yes, we ignore them ;) Richard. > Honza > > > > Thanks, > > Richard. > > > > > Honza > > > > > > * lto-cgraph.c (input_overwrite_node): Check that partitioning > > > flags are set only during streaming. > > > * ipa.c (process_references, walk_polymorphic_call_targets, > > > symtab_remove_unreachable_nodes): Drop bodies of always inline > > > after early inlining. > > > (symtab_remove_unreachable_nodes): Remove always_inline attribute. > > > > > > * gcc.dg/lto/pr59626_0.c: New testcase. > > > * gcc.dg/lto/pr59626_1.c: New testcase. > > > Index: lto-cgraph.c > > > =================================================================== > > > --- lto-cgraph.c (revision 209062) > > > +++ lto-cgraph.c (working copy) > > > @@ -1001,6 +1001,9 @@ input_overwrite_node (struct lto_file_de > > > node->thunk.thunk_p = bp_unpack_value (bp, 1); > > > node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution, > > > LDPR_NUM_KNOWN); > > > + gcc_assert (flag_ltrans > > > + || (!node->in_other_partition > > > + && !node->used_from_other_partition)); > > > } > > > > > > /* Return string alias is alias of. */ > > > @@ -1169,6 +1172,9 @@ input_varpool_node (struct lto_file_decl > > > node->same_comdat_group = (symtab_node *) (intptr_t) ref; > > > node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution, > > > LDPR_NUM_KNOWN); > > > + gcc_assert (flag_ltrans > > > + || (!node->in_other_partition > > > + && !node->used_from_other_partition)); > > > > > > return node; > > > } > > > Index: ipa.c > > > =================================================================== > > > --- ipa.c (revision 209062) > > > +++ ipa.c (working copy) > > > @@ -139,7 +139,10 @@ process_references (struct ipa_ref_list > > > > > > if (node->definition && !node->in_other_partition > > > && ((!DECL_EXTERNAL (node->decl) || node->alias) > > > - || (before_inlining_p > > > + || (((before_inlining_p > > > + && (cgraph_state < CGRAPH_STATE_IPA_SSA > > > + || !lookup_attribute ("always_inline", > > > + DECL_ATTRIBUTES (node->decl))))) > > > /* We use variable constructors during late complation for > > > constant folding. Keep references alive so partitioning > > > knows about potential references. */ > > > @@ -191,7 +194,10 @@ walk_polymorphic_call_targets (pointer_s > > > /* Prior inlining, keep alive bodies of possible targets for > > > devirtualization. */ > > > if (n->definition > > > - && before_inlining_p) > > > + && (before_inlining_p > > > + && (cgraph_state < CGRAPH_STATE_IPA_SSA > > > + || !lookup_attribute ("always_inline", > > > + DECL_ATTRIBUTES (n->decl))))) > > > pointer_set_insert (reachable, n); > > > > > > /* Even after inlining we want to keep the possible targets in the > > > @@ -491,6 +497,12 @@ symtab_remove_unreachable_nodes (bool be > > > node->alias = false; > > > node->thunk.thunk_p = false; > > > node->weakref = false; > > > + /* After early inlining we drop always_inline attributes on > > > + bodies of functions that are still referenced (have their > > > + address taken). */ > > > + DECL_ATTRIBUTES (node->decl) > > > + = remove_attribute ("always_inline", > > > + DECL_ATTRIBUTES (node->decl)); > > > if (!node->in_other_partition) > > > node->local.local = false; > > > cgraph_node_remove_callees (node); > > > Index: testsuite/gcc.dg/lto/pr59626_1.c > > > =================================================================== > > > --- testsuite/gcc.dg/lto/pr59626_1.c (revision 0) > > > +++ testsuite/gcc.dg/lto/pr59626_1.c (revision 0) > > > @@ -0,0 +1,4 @@ > > > +int bar (int (*fn)(const char *)) > > > +{ > > > + return fn ("0"); > > > +} > > > Index: testsuite/gcc.dg/lto/pr59626_0.c > > > =================================================================== > > > --- testsuite/gcc.dg/lto/pr59626_0.c (revision 0) > > > +++ testsuite/gcc.dg/lto/pr59626_0.c (revision 0) > > > @@ -0,0 +1,15 @@ > > > +/* { dg-lto-do run } */ > > > + > > > +int __atoi (const char *) __asm__("atoi"); > > > +extern inline __attribute__((always_inline,gnu_inline)) > > > +int atoi (const char *x) > > > +{ > > > + return __atoi (x); > > > +} > > > + > > > +int bar (int (*)(const char *)); > > > + > > > +int main() > > > +{ > > > + return bar (atoi); > > > +} > > > > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE / SUSE Labs > > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > > GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer > >
On Fri, 4 Apr 2014 08:18:26 +0200 Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, > here is an updated version of my earlier ipa.c change. It turns out that the > problem was that I did not drop always_inline. > In this version I just drop always_inline attribute on all functions whose > body is removed. The patch will affect non-LTO compilation, too, but IMO > only by making us to not inline&diagnose the cases where indirect call to > always inline is turned direct in between early opts and inline. Does that > seem acceptable? (I personally would preffer this over inventing yet another > way to special case always_inline for LTO only; we never made any strong > promises on always_inline and indirect calls) > > Honza > > * lto-cgraph.c (input_overwrite_node): Check that partitioning > flags are set only during streaming. > * ipa.c (process_references, walk_polymorphic_call_targets, > symtab_remove_unreachable_nodes): Drop bodies of always inline > after early inlining. > (symtab_remove_unreachable_nodes): Remove always_inline attribute. > > * gcc.dg/lto/pr59626_0.c: New testcase. > * gcc.dg/lto/pr59626_1.c: New testcase. Would it be possible to backport this to 4.8? It fixes a number of issues we're seeing, including a couple unrelated to LTO (eg. PR50506).
On Tue, Apr 8, 2014 at 8:12 AM, Ryan Hill <rhill@gentoo.org> wrote: > On Fri, 4 Apr 2014 08:18:26 +0200 > Jan Hubicka <hubicka@ucw.cz> wrote: > >> Hi, >> here is an updated version of my earlier ipa.c change. It turns out that the >> problem was that I did not drop always_inline. >> In this version I just drop always_inline attribute on all functions whose >> body is removed. The patch will affect non-LTO compilation, too, but IMO >> only by making us to not inline&diagnose the cases where indirect call to >> always inline is turned direct in between early opts and inline. Does that >> seem acceptable? (I personally would preffer this over inventing yet another >> way to special case always_inline for LTO only; we never made any strong >> promises on always_inline and indirect calls) >> >> Honza >> >> * lto-cgraph.c (input_overwrite_node): Check that partitioning >> flags are set only during streaming. >> * ipa.c (process_references, walk_polymorphic_call_targets, >> symtab_remove_unreachable_nodes): Drop bodies of always inline >> after early inlining. >> (symtab_remove_unreachable_nodes): Remove always_inline attribute. >> >> * gcc.dg/lto/pr59626_0.c: New testcase. >> * gcc.dg/lto/pr59626_1.c: New testcase. > > Would it be possible to backport this to 4.8? It fixes a number of issues > we're seeing, including a couple unrelated to LTO (eg. PR50506). Let it have some burn-in on 4.9 please. It may cause unexpected behavior for people relying on always-inlining through indirect function calls. Richard. > > -- > Ryan Hill psn: dirtyepic_sk > gcc-porting/toolchain/wxwidgets @ gentoo.org > > 47C3 6D62 4864 0E49 8E9E 7F92 ED38 BD49 957A 8463
Index: lto-cgraph.c =================================================================== --- lto-cgraph.c (revision 209062) +++ lto-cgraph.c (working copy) @@ -1001,6 +1001,9 @@ input_overwrite_node (struct lto_file_de node->thunk.thunk_p = bp_unpack_value (bp, 1); node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution, LDPR_NUM_KNOWN); + gcc_assert (flag_ltrans + || (!node->in_other_partition + && !node->used_from_other_partition)); } /* Return string alias is alias of. */ @@ -1169,6 +1172,9 @@ input_varpool_node (struct lto_file_decl node->same_comdat_group = (symtab_node *) (intptr_t) ref; node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution, LDPR_NUM_KNOWN); + gcc_assert (flag_ltrans + || (!node->in_other_partition + && !node->used_from_other_partition)); return node; } Index: ipa.c =================================================================== --- ipa.c (revision 209062) +++ ipa.c (working copy) @@ -139,7 +139,10 @@ process_references (struct ipa_ref_list if (node->definition && !node->in_other_partition && ((!DECL_EXTERNAL (node->decl) || node->alias) - || (before_inlining_p + || (((before_inlining_p + && (cgraph_state < CGRAPH_STATE_IPA_SSA + || !lookup_attribute ("always_inline", + DECL_ATTRIBUTES (node->decl))))) /* We use variable constructors during late complation for constant folding. Keep references alive so partitioning knows about potential references. */ @@ -191,7 +194,10 @@ walk_polymorphic_call_targets (pointer_s /* Prior inlining, keep alive bodies of possible targets for devirtualization. */ if (n->definition - && before_inlining_p) + && (before_inlining_p + && (cgraph_state < CGRAPH_STATE_IPA_SSA + || !lookup_attribute ("always_inline", + DECL_ATTRIBUTES (n->decl))))) pointer_set_insert (reachable, n); /* Even after inlining we want to keep the possible targets in the @@ -491,6 +497,12 @@ symtab_remove_unreachable_nodes (bool be node->alias = false; node->thunk.thunk_p = false; node->weakref = false; + /* After early inlining we drop always_inline attributes on + bodies of functions that are still referenced (have their + address taken). */ + DECL_ATTRIBUTES (node->decl) + = remove_attribute ("always_inline", + DECL_ATTRIBUTES (node->decl)); if (!node->in_other_partition) node->local.local = false; cgraph_node_remove_callees (node); Index: testsuite/gcc.dg/lto/pr59626_1.c =================================================================== --- testsuite/gcc.dg/lto/pr59626_1.c (revision 0) +++ testsuite/gcc.dg/lto/pr59626_1.c (revision 0) @@ -0,0 +1,4 @@ +int bar (int (*fn)(const char *)) +{ + return fn ("0"); +} Index: testsuite/gcc.dg/lto/pr59626_0.c =================================================================== --- testsuite/gcc.dg/lto/pr59626_0.c (revision 0) +++ testsuite/gcc.dg/lto/pr59626_0.c (revision 0) @@ -0,0 +1,15 @@ +/* { dg-lto-do run } */ + +int __atoi (const char *) __asm__("atoi"); +extern inline __attribute__((always_inline,gnu_inline)) +int atoi (const char *x) +{ + return __atoi (x); +} + +int bar (int (*)(const char *)); + +int main() +{ + return bar (atoi); +}