| Message ID | 20191030092236.og3pjotzqrzsaifs@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Watch for missing summaries even more | expand |
Hi, On Wed, Oct 30 2019, Jan Hubicka wrote: > Hi, > this patch fixes another place we may have missing argument summary. > Here the situation is that the call site being inlined has no jump > functions while function which is being inlines has another call with > jump function. This can validly happen when we inline into functions > with indirect inlining and ipa-cp disabled but I am not 100% why it > happens i.e. during Firefox builds. Martin, do you have any ideas? No, not without seeing what is going on. I think we compute jump functions also when we are inlining and IPA-CP is disabled, by the way. It looks like ipa_compute_jump_functions_for_edge was never called on that edge, so if the problem appeared in a non-LTO context the following might help? And I assume we want it either way, so OK for trunk after a bootstrap&testing? Martin diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 0dd73561419..10fe1bc929f 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -2040,7 +2040,7 @@ ipa_compute_jump_functions_for_bb (struct ipa_func_body_info *fbi, basic_block b if (callee) { - callee->ultimate_alias_target (); + callee = callee->ultimate_alias_target (); /* We do not need to bother analyzing calls to unknown functions unless they may become known during lto/whopr. */ if (!callee->definition && !flag_lto)
On Wed, Oct 30, 2019 at 1:06 PM Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > On Wed, Oct 30 2019, Jan Hubicka wrote: > > Hi, > > this patch fixes another place we may have missing argument summary. > > Here the situation is that the call site being inlined has no jump > > functions while function which is being inlines has another call with > > jump function. This can validly happen when we inline into functions > > with indirect inlining and ipa-cp disabled but I am not 100% why it > > happens i.e. during Firefox builds. Martin, do you have any ideas? > > No, not without seeing what is going on. I think we compute jump > functions also when we are inlining and IPA-CP is disabled, by the way. > > It looks like ipa_compute_jump_functions_for_edge was never called on > that edge, so if the problem appeared in a non-LTO context the following > might help? And I assume we want it either way, so OK for trunk after a > bootstrap&testing? > > Martin > > > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > index 0dd73561419..10fe1bc929f 100644 > --- a/gcc/ipa-prop.c > +++ b/gcc/ipa-prop.c > @@ -2040,7 +2040,7 @@ ipa_compute_jump_functions_for_bb (struct ipa_func_body_info *fbi, basic_block b > > if (callee) > { > - callee->ultimate_alias_target (); > + callee = callee->ultimate_alias_target (); so that either was dead code or your fix is obvious ;) So - OK. Thanks, Richard. > /* We do not need to bother analyzing calls to unknown functions > unless they may become known during lto/whopr. */ > if (!callee->definition && !flag_lto) >
Hi again, now also CCing the mailing list, On Wed, Oct 30 2019, Jan Hubicka wrote: >> Hi, >> >> On Wed, Oct 30 2019, Jan Hubicka wrote: >> > Hi, >> > this patch fixes another place we may have missing argument summary. >> > Here the situation is that the call site being inlined has no jump >> > functions while function which is being inlines has another call with >> > jump function. This can validly happen when we inline into functions >> > with indirect inlining and ipa-cp disabled but I am not 100% why it >> > happens i.e. during Firefox builds. Martin, do you have any ideas? >> >> No, not without seeing what is going on. I think we compute jump >> functions also when we are inlining and IPA-CP is disabled, by the way. >> >> It looks like ipa_compute_jump_functions_for_edge was never called on >> that edge, so if the problem appeared in a non-LTO context the following >> might help? And I assume we want it either way, so OK for trunk after a >> bootstrap&testing? > > This is OK, though I think it will only result in fewer summaries, > because aliases are definitions, but that code is obviously broken. > I have just committed it. Looking at PR 92278, I think I found the real problem. In ipa_read_edge_info, you added code to throw away jump functions of edges that do not pass possibly_call_in_translation_unit_p() test. But that predicate incorrectly - or at least I think so, see below - returns false for edges leading to interposable symbols. The reason why I think it is incorrect is because a node which is considered interposable before merging can apparently later on be upgraded to a local one by ipa-visibility. I am testing the following fix, is it OK if it passes? The testcase passes a version script to the linker, I guess our LTO testsuite does not support that, does it? Thanks, Martin 2019-10-30 Martin Jambor <mjambor@suse.cz> ipa/92278 * cgraph.c (cgraph_edge::possibly_call_in_translation_unit_p): Fix availability comparison. --- gcc/cgraph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/cgraph.c b/gcc/cgraph.c index d47d4128b1c..8057ccdb7c0 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -3813,7 +3813,7 @@ cgraph_edge::possibly_call_in_translation_unit_p (void) if (node->previous_sharing_asm_name) node = symtab_node::get_for_asmname (DECL_ASSEMBLER_NAME (callee->decl)); gcc_assert (TREE_PUBLIC (node->decl)); - return node->get_availability () >= AVAIL_AVAILABLE; + return node->get_availability () >= AVAIL_INTERPOSABLE; } /* A stashed copy of "symtab" for use by selftest::symbol_table_test.
Hi, On Wed, Oct 30 2019, Jan Hubicka wrote: >> >> Looking at PR 92278, I think I found the real problem. In >> ipa_read_edge_info, you added code to throw away jump functions of edges >> that do not pass possibly_call_in_translation_unit_p() test. But that >> predicate incorrectly - or at least I think so, see below - returns >> false for edges leading to interposable symbols. The reason why I think >> it is incorrect is because a node which is considered interposable >> before merging can apparently later on be upgraded to a local one by >> ipa-visibility. >> >> I am testing the following fix, is it OK if it passes? >> >> The testcase passes a version script to the linker, I guess our LTO >> testsuite does not support that, does it? >> >> Thanks, >> >> Martin >> >> >> 2019-10-30 Martin Jambor <mjambor@suse.cz> >> >> ipa/92278 >> * cgraph.c (cgraph_edge::possibly_call_in_translation_unit_p): Fix >> availability comparison. > > yes, this is OK - thanks for looking into this! > While jump functions are interesting only for available symbols > interposable symbols may be promoted to them based on resolution info. > > Lets see how much extra memory this will cost. If it is too bad I can > add resolution info logic but duplicating it from ipa-visibility is not > cool. OK, I have committed the patch. I wonder whether we want to revert the early exit in update_jump_functions_after_inlining - or replace it with a gcc_checking_assert - now. Anyway, thanks, Martin > > Honza >> --- >> gcc/cgraph.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/cgraph.c b/gcc/cgraph.c >> index d47d4128b1c..8057ccdb7c0 100644 >> --- a/gcc/cgraph.c >> +++ b/gcc/cgraph.c >> @@ -3813,7 +3813,7 @@ cgraph_edge::possibly_call_in_translation_unit_p (void) >> if (node->previous_sharing_asm_name) >> node = symtab_node::get_for_asmname (DECL_ASSEMBLER_NAME (callee->decl)); >> gcc_assert (TREE_PUBLIC (node->decl)); >> - return node->get_availability () >= AVAIL_AVAILABLE; >> + return node->get_availability () >= AVAIL_INTERPOSABLE; >> } >> >> /* A stashed copy of "symtab" for use by selftest::symbol_table_test. >> -- >> 2.23.0 >>
> Hi, > > On Wed, Oct 30 2019, Jan Hubicka wrote: > >> > >> Looking at PR 92278, I think I found the real problem. In > >> ipa_read_edge_info, you added code to throw away jump functions of edges > >> that do not pass possibly_call_in_translation_unit_p() test. But that > >> predicate incorrectly - or at least I think so, see below - returns > >> false for edges leading to interposable symbols. The reason why I think > >> it is incorrect is because a node which is considered interposable > >> before merging can apparently later on be upgraded to a local one by > >> ipa-visibility. > >> > >> I am testing the following fix, is it OK if it passes? > >> > >> The testcase passes a version script to the linker, I guess our LTO > >> testsuite does not support that, does it? > >> > >> Thanks, > >> > >> Martin > >> > >> > >> 2019-10-30 Martin Jambor <mjambor@suse.cz> > >> > >> ipa/92278 > >> * cgraph.c (cgraph_edge::possibly_call_in_translation_unit_p): Fix > >> availability comparison. > > > > yes, this is OK - thanks for looking into this! > > While jump functions are interesting only for available symbols > > interposable symbols may be promoted to them based on resolution info. > > > > Lets see how much extra memory this will cost. If it is too bad I can > > add resolution info logic but duplicating it from ipa-visibility is not > > cool. > > OK, I have committed the patch. I wonder whether we want to revert the > early exit in update_jump_functions_after_inlining - or replace it with > a gcc_checking_assert - now. I do not think we can revert it - we want to be able to inline functions with no jump functions (-O0 and -fno-ipa-cp -fno-indirect-inlining). Having assert would be useful (we had another two bugs in this direction I fixed during weekend) - we probably could check that if summary is missing then the function must be compiled without those flags? Honza > > Anyway, thanks, > > Martin > > > > > > Honza > >> --- > >> gcc/cgraph.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/gcc/cgraph.c b/gcc/cgraph.c > >> index d47d4128b1c..8057ccdb7c0 100644 > >> --- a/gcc/cgraph.c > >> +++ b/gcc/cgraph.c > >> @@ -3813,7 +3813,7 @@ cgraph_edge::possibly_call_in_translation_unit_p (void) > >> if (node->previous_sharing_asm_name) > >> node = symtab_node::get_for_asmname (DECL_ASSEMBLER_NAME (callee->decl)); > >> gcc_assert (TREE_PUBLIC (node->decl)); > >> - return node->get_availability () >= AVAIL_AVAILABLE; > >> + return node->get_availability () >= AVAIL_INTERPOSABLE; > >> } > >> > >> /* A stashed copy of "symtab" for use by selftest::symbol_table_test. > >> -- > >> 2.23.0 > >>
Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 277573) +++ ipa-prop.c (working copy) @@ -2660,6 +2660,11 @@ update_jump_functions_after_inlining (st for (i = 0; i < count; i++) { struct ipa_jump_func *dst = ipa_get_ith_jump_func (args, i); + if (!top) + { + ipa_set_jf_unknown (dst); + continue; + } class ipa_polymorphic_call_context *dst_ctx = ipa_get_ith_polymorhic_call_context (args, i);