ipa: Fix removal of multi-target speculation

Message ID 20200129114659.GN83181@kam.mff.cuni.cz
State New
Headers show
Series
  • ipa: Fix removal of multi-target speculation
Related show

Commit Message

Jan Hubicka Jan. 29, 2020, 11:46 a.m.
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.

Comments

Martin Liška Jan. 29, 2020, 12:29 p.m. | #1
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];

>   	}

>       }

>   }

>
Jan Hubicka Jan. 29, 2020, 12:39 p.m. | #2
> > 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
Martin Liška Jan. 29, 2020, 12:41 p.m. | #3
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

>

Patch

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
@@ -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
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];
 	}
     }
 }