Watch for missing summaries even more

Message ID 20191030092236.og3pjotzqrzsaifs@kam.mff.cuni.cz
State New
Headers show
Series
  • Watch for missing summaries even more
Related show

Commit Message

Jan Hubicka Oct. 30, 2019, 9:22 a.m.
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?

Honza

	* ipa-prop.c (update_jump_functions_after_inlining):
	Watch for missing summaries.

Comments

Martin Jambor Oct. 30, 2019, 12:06 p.m. | #1
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)
Richard Biener Oct. 30, 2019, 2:05 p.m. | #2
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)

>
Martin Jambor Oct. 30, 2019, 2:33 p.m. | #3
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.
-- 
2.23.0
Martin Jambor Oct. 30, 2019, 3:53 p.m. | #4
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

>>
Jan Hubicka Oct. 30, 2019, 3:54 p.m. | #5
> 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

> >>

Patch

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);