ipa: Fix removal of multi-target speculation

Hi,
this fixes indirect call speculation ICE Martin (Liska) reduced from
spec GCC.  The problem here was in resolve_speculation that if called to
resolve the second edge in multi-target speculative call resolved first
on instead.  This is bug I did not catch during the API change.
Other problem is that redirect_call_stmt_to_callee if called from
inliner is called on direct part of speculative call while it expects
indirect.  This is checking only failure but rahter than removing sanity
check I switch to indirect edge.

profiled-lto-bootstrapped x86_64-linux and comitted.

Honza

2020-01-29  Jan Hubicka  <hubicka@ucw.cz>

	* cgraph.c (cgraph_edge::resolve_speculation): Only lookup direct edge
	if called on indirect edge.
	(cgraph_edge::redirect_call_stmt_to_callee): Lookup indirect edge of
	speculative call if needed.

gcc/testsuite/ChangeLog:

2020-01-29  Jan Hubicka  <hubicka@ucw.cz>

	* gcc.dg/tree-prof/indir-call-prof-2.c: New testcase.

On 1/29/20 12:46 PM, Jan Hubicka wrote:
> Hi,
> this fixes indirect call speculation ICE Martin (Liska) reduced from
> spec GCC.  The problem here was in resolve_speculation that if called to
> resolve the second edge in multi-target speculative call resolved first
> on instead.  This is bug I did not catch during the API change.
> Other problem is that redirect_call_stmt_to_callee if called from
> inliner is called on direct part of speculative call while it expects
> indirect.  This is checking only failure but rahter than removing sanity
> check I switch to indirect edge.
> 
> profiled-lto-bootstrapped x86_64-linux and comitted.

Thanks for the patch ...

> 
> Honza
> 
> 2020-01-29  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* cgraph.c (cgraph_edge::resolve_speculation): Only lookup direct edge
> 	if called on indirect edge.
> 	(cgraph_edge::redirect_call_stmt_to_callee): Lookup indirect edge of
> 	speculative call if needed.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-01-29  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* gcc.dg/tree-prof/indir-call-prof-2.c: New testcase.
> 
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 3e50b0bc380..294b2d392fd 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -1189,7 +1189,10 @@ cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl)
>     ipa_ref *ref;
>   
>     gcc_assert (edge->speculative && (!callee_decl || edge->callee));
> -  e2 = edge->first_speculative_call_target ();
> +  if (!edge->callee)
> +    e2 = edge->first_speculative_call_target ();
> +  else
> +    e2 = edge;
>     ref = e2->speculative_call_target_ref ();
>     edge = edge->speculative_call_indirect_edge ();
>     if (!callee_decl
> @@ -1364,7 +1367,8 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
>         /* If there already is an direct call (i.e. as a result of inliner's
>   	 substitution), forget about speculating.  */
>         if (decl)
> -	e = make_direct (e, cgraph_node::get (decl));
> +	e = make_direct (e->speculative_call_indirect_edge (),
> +			 cgraph_node::get (decl));
>         else
>   	{
>   	  /* Be sure we redirect all speculative targets before poking
> diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
> index 61612b5b628..bbba0521018 100644
> --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
> +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c

This should be a new file.

> @@ -1,25 +1,28 @@
>   /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */
>   volatile int one;
> -static
> -int add1 (int val)
> +static int
> +add1 (int val)
>   {
> -  return val+=one;
> +  return val += one;
>   }
> -static
> -int sub1 (int val)
> +
> +static int
> +sub1 (int val)
>   {
> -  return val-=one;
> +  return val -= one;
>   }
> -static
> -int do_op (int val, int (*fnptr) (int))
> +
> +static int
> +do_op (int val, int (*fnptr) (int))
>   {
>     return fnptr (val);
>   }
> +
>   int
> -main(void)
> +main (void)
>   {
> -  int i,val;
> -  for (i=0;i<100000;i++)
> +  int i, val = 0;
> +  for (i = 0; i < 100000; i++)
>       {
>         val = do_op (val, add1);
>         val = do_op (val, sub1);
> diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c

... and this should not be installed.

Martin

> index 19b8ee72ae9..801037d0d64 100644
> --- a/libgcc/libgcov-merge.c
> +++ b/libgcc/libgcov-merge.c
> @@ -115,7 +115,7 @@ merge_topn_values_set (gcov_type *counters)
>   	continue;
>   
>         unsigned j;
> -      int slot = -1;
> +      int slot = 0;
>   
>         for (j = 0; j < GCOV_TOPN_VALUES; j++)
>   	{
> @@ -124,24 +124,15 @@ merge_topn_values_set (gcov_type *counters)
>   	      counters[2 * j + 1] += read_counters[2 * i + 1];
>   	      break;
>   	    }
> -	  else if (counters[2 * j + 1] == 0)
> +	  if (counters[2 * j + 1] < counters[2 * slot + 1])
>   	    slot = j;
>   	}
>   
> -      if (j == GCOV_TOPN_VALUES)
> +      if (j == GCOV_TOPN_VALUES
> +	  && counters[2 * slot + 1] < read_counters[2 * i + 1])
>   	{
> -	  if (slot > 0)
> -	    {
> -	      /* If we found empty slot, add the value.  */
> -	      counters[2 * slot] = read_counters[2 * i];
> -	      counters[2 * slot + 1] = read_counters[2 * i + 1];
> -	    }
> -	  else
> -	    {
> -	      /* We haven't found a slot, bail out.  */
> -	      counters[1] = -1;
> -	      return;
> -	    }
> +	  counters[2 * slot] = read_counters[2 * i];
> +	  counters[2 * slot + 1] = read_counters[2 * i + 1];
>   	}
>       }
>   }
>

> > diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
> > index 61612b5b628..bbba0521018 100644
> > --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
> > +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
> 
> This should be a new file.
Sorry, git is still not my friend. I am used to add new files when I
star them, then fix them and then expect them as new files in diff.
git insists on diffing over the original file I added.
> 
> > @@ -1,25 +1,28 @@
> >   /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */
> >   volatile int one;
> > -static
> > -int add1 (int val)
> > +static int
> > +add1 (int val)
> >   {
> > -  return val+=one;
> > +  return val += one;
> >   }
> > -static
> > -int sub1 (int val)
> > +
> > +static int
> > +sub1 (int val)
> >   {
> > -  return val-=one;
> > +  return val -= one;
> >   }
> > -static
> > -int do_op (int val, int (*fnptr) (int))
> > +
> > +static int
> > +do_op (int val, int (*fnptr) (int))
> >   {
> >     return fnptr (val);
> >   }
> > +
> >   int
> > -main(void)
> > +main (void)
> >   {
> > -  int i,val;
> > -  for (i=0;i<100000;i++)
> > +  int i, val = 0;
> > +  for (i = 0; i < 100000; i++)
> >       {
> >         val = do_op (val, add1);
> >         val = do_op (val, sub1);
> > diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c
> 
> ... and this should not be installed.

.. and this was accidentally included in patch (it is hack to disable
reproducibility).
Thanks for pointing this out.

Honza

On 1/29/20 1:39 PM, Jan Hubicka wrote:
>>> diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
>>> index 61612b5b628..bbba0521018 100644
>>> --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
>>> +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c
>>
>> This should be a new file.
> Sorry, git is still not my friend. I am used to add new files when I
> star them, then fix them and then expect them as new files in diff.

Then it's a diff against a file in git index. Then you need to do something like:
$ git diff HEAD

> git insists on diffing over the original file I added.

Anyway, I would definitely recommend to make a commit and then send
a result with git format-patch -1 HEAD. That's more safe than a diff.

Martin

>>
>>> @@ -1,25 +1,28 @@
>>>    /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */
>>>    volatile int one;
>>> -static
>>> -int add1 (int val)
>>> +static int
>>> +add1 (int val)
>>>    {
>>> -  return val+=one;
>>> +  return val += one;
>>>    }
>>> -static
>>> -int sub1 (int val)
>>> +
>>> +static int
>>> +sub1 (int val)
>>>    {
>>> -  return val-=one;
>>> +  return val -= one;
>>>    }
>>> -static
>>> -int do_op (int val, int (*fnptr) (int))
>>> +
>>> +static int
>>> +do_op (int val, int (*fnptr) (int))
>>>    {
>>>      return fnptr (val);
>>>    }
>>> +
>>>    int
>>> -main(void)
>>> +main (void)
>>>    {
>>> -  int i,val;
>>> -  for (i=0;i<100000;i++)
>>> +  int i, val = 0;
>>> +  for (i = 0; i < 100000; i++)
>>>        {
>>>          val = do_op (val, add1);
>>>          val = do_op (val, sub1);
>>> diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c
>>
>> ... and this should not be installed.
> 
> .. and this was accidentally included in patch (it is hack to disable
> reproducibility).
> Thanks for pointing this out.
> 
> Honza
>