[PATCH] PR80101: Fix ICE in store_data_bypass_p

[PATCH] PR80101: Fix ICE in store_data_bypass_p

[Kelvin Nilsen]

[This is a repost of a patch previously posted on 3/29/2017.
Eric, I hope you might consider that this falls within your scope
of maintenance.  Thanks.]

This problem reports an assertion error when certain rtl expressions
which are not eligible as producers or consumers of a store bypass
optimization are passed as arguments to the store_data_bypass_p
function.  The proposed patch returns false from store_data_bypass_p
rather than terminating with an assertion error.  False indicates that
the passed arguments are not eligible for the store bypass scheduling
optimization.

The patch has been boostrapped without regressions on
powerpc64le-unknown-linux-gnu.  Is this ok for the trunk?

gcc/ChangeLog:

2017-03-29  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	PR target/80101
	* recog.c (store_data_bypass_p): Rather than terminate with
	assertion error, return false if either function argument is not a
	single_set or a PARALLEL with SETs inside.

gcc/testsuite/ChangeLog:

2017-03-29  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	PR target/80101
	* gcc.target/powerpc/pr80101-1.c: New test.


Index: gcc/recog.c
===================================================================
--- gcc/recog.c	(revision 246469)
+++ gcc/recog.c	(working copy)
@@ -3663,9 +3663,12 @@ peephole2_optimize (void)

 /* Common predicates for use with define_bypass.  */

-/* True if the dependency between OUT_INSN and IN_INSN is on the store
-   data not the address operand(s) of the store.  IN_INSN and OUT_INSN
-   must be either a single_set or a PARALLEL with SETs inside.  */
+/* Returns true if the dependency between OUT_INSN and IN_INSN is on
+   the stored data, false if there is no dependency.  Note that a
+   consumer instruction that loads only the address (rather than the
+   value) stored by a producer instruction does not represent a
+   dependency.  If IN_INSN or OUT_INSN are not a single_set or a
+   PARALLEL with SETs inside, this function returns false.  */

 int
 store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn)
@@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
             if (GET_CODE (out_exp) == CLOBBER)
               continue;

-            gcc_assert (GET_CODE (out_exp) == SET);
+	    if (GET_CODE (out_exp) != SET)
+	      return false;

             if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
               return false;
@@ -3711,7 +3715,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
   else
     {
       in_pat = PATTERN (in_insn);
-      gcc_assert (GET_CODE (in_pat) == PARALLEL);
+      if (GET_CODE (in_pat) != PARALLEL)
+	return false;

       for (i = 0; i < XVECLEN (in_pat, 0); i++)
 	{
@@ -3720,7 +3725,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
 	  if (GET_CODE (in_exp) == CLOBBER)
 	    continue;

-	  gcc_assert (GET_CODE (in_exp) == SET);
+	  if (GET_CODE (in_exp) != SET)
+	    return false;

 	  if (!MEM_P (SET_DEST (in_exp)))
 	    return false;
@@ -3734,7 +3740,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
           else
             {
               out_pat = PATTERN (out_insn);
-              gcc_assert (GET_CODE (out_pat) == PARALLEL);
+	      if (GET_CODE (out_pat) != PARALLEL)
+		return false;

               for (j = 0; j < XVECLEN (out_pat, 0); j++)
                 {
@@ -3743,7 +3750,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
                   if (GET_CODE (out_exp) == CLOBBER)
                     continue;

-                  gcc_assert (GET_CODE (out_exp) == SET);
+		  if (GET_CODE (out_exp) != SET)
+		    return false;

                   if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_exp)))
                     return false;
Index: gcc/testsuite/gcc.target/powerpc/pr80101-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80101-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80101-1.c	(working copy)
@@ -0,0 +1,22 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power6" } } */
+/* { dg-require-effective-target dfp_hw } */
+/* { dg-options "-mcpu=power6 -mno-sched-epilog -Ofast" } */
+
+/* Prior to resolving PR 80101, this test case resulted in an internal
+   compiler error.  The role of this test program is to assure that
+   dejagnu's "test for excess errors" does not find any.  */
+
+int b;
+
+void e ();
+
+int c ()
+{
+  struct
+  {
+    int a[b];
+  } d;
+  if (d.a[0])
+    e ();
+}

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Eric Botcazou]

> [This is a repost of a patch previously posted on 3/29/2017.
> Eric, I hope you might consider that this falls within your scope
> of maintenance.  Thanks.]

My viewpoint is that it's better to keep the assertions and fix the back-end 
instead, which looks rather straightforward.

-- 
Eric Botcazou

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Segher Boessenkool]

On Thu, Apr 06, 2017 at 10:05:54PM +0200, Eric Botcazou wrote:
> > [This is a repost of a patch previously posted on 3/29/2017.
> > Eric, I hope you might consider that this falls within your scope
> > of maintenance.  Thanks.]
> 
> My viewpoint is that it's better to keep the assertions and fix the back-end 
> instead, which looks rather straightforward.

The only straightforward way I see is to use a rs6000_store_data_bypass_p
instead, which would be doing the same thing.  :-(

[ It of course is easy to workaround the specific problem in the testcase,
but that does not solve any of the underlying problems. ]


Segher

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Eric Botcazou]

> The only straightforward way I see is to use a rs6000_store_data_bypass_p
> instead, which would be doing the same thing.  :-(

Why not just change the type of the blockage instruction as you suggested?

-- 
Eric Botcazou

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Segher Boessenkool]

On Fri, Apr 07, 2017 at 08:54:01AM +0200, Eric Botcazou wrote:
> > The only straightforward way I see is to use a rs6000_store_data_bypass_p
> > instead, which would be doing the same thing.  :-(
> 
> Why not just change the type of the blockage instruction as you suggested?

That works for this case, sure, but it won't solve the underlying
problem.

The default instruction type for rs6000 is "integer".  Many instructions
have this type: some bswap, register moves, load address/immediate,
mtvrsave, nop, and many things that are split; and that is just those
that have type "*" (so are easy to search for), not all those that do
no explicitly have a type attribute (including blockage).

Now, the power6 scheduler defines a bypass from power6-integer (which is
types "integer" as well as "add" and "logical") to stores.  And then
store_data_bypass_p ICEs because not all "integer" insns are a SET or
a PARALLEL of SETs.

So, changing the type involves changing the default to something else,
changing all pipeline descriptions to do the same with that type as it
does with "integer", checking all patterns that are type "integer" to
see if they should be that new type or not.  Or we could just change
"blockage" and wait for the next bug report.

Alternatively, we can arrange for the bypass functions to not ICE.  We
can do that specific to these rs6000 pipeline descriptions, by having
our own version of store_data_bypass_p; or we can make that function
work for all insns (its definition works fine for insn pairs where
not both the producer and consumer are SETs).  That's what Kelvin's
patch does.  What is the value in ICEing here?


Segher

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Eric Botcazou]

> Or we could just change "blockage" and wait for the next bug report.

That's my suggestion, yes.

> Alternatively, we can arrange for the bypass functions to not ICE.  We
> can do that specific to these rs6000 pipeline descriptions, by having
> our own version of store_data_bypass_p; or we can make that function
> work for all insns (its definition works fine for insn pairs where
> not both the producer and consumer are SETs).  That's what Kelvin's
> patch does.  What is the value in ICEing here?

Telling the back-end writer that something may be wrong somewhere instead of 
silently accepting nonsense?  How long have all the assertions been there?

-- 
Eric Botcazou

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Segher Boessenkool]

On Fri, Apr 07, 2017 at 10:39:03AM +0200, Eric Botcazou wrote:
> > Or we could just change "blockage" and wait for the next bug report.
> 
> That's my suggestion, yes.
> 
> > Alternatively, we can arrange for the bypass functions to not ICE.  We
> > can do that specific to these rs6000 pipeline descriptions, by having
> > our own version of store_data_bypass_p; or we can make that function
> > work for all insns (its definition works fine for insn pairs where
> > not both the producer and consumer are SETs).  That's what Kelvin's
> > patch does.  What is the value in ICEing here?
> 
> Telling the back-end writer that something may be wrong somewhere instead of 
> silently accepting nonsense?

Why is it nonsense?  The predicate gives the answer to the question
"given these insns A and B, does A feed data that B stores in memory".
That is a perfectly valid question to ask of any two insns.

> How long have all the assertions been there?

There are workarounds to this problem as well: mips_store_data_bypass_p,
added in 2006.  mep_store_data_bypass_p, added in 2009 (the port has
been removed since then, of course).


Segher

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Fri, Apr 07, 2017 at 10:39:03AM +0200, Eric Botcazou wrote:
>> > Or we could just change "blockage" and wait for the next bug report.
>> 
>> That's my suggestion, yes.
>> 
>> > Alternatively, we can arrange for the bypass functions to not ICE.  We
>> > can do that specific to these rs6000 pipeline descriptions, by having
>> > our own version of store_data_bypass_p; or we can make that function
>> > work for all insns (its definition works fine for insn pairs where
>> > not both the producer and consumer are SETs).  That's what Kelvin's
>> > patch does.  What is the value in ICEing here?
>> 
>> Telling the back-end writer that something may be wrong somewhere instead of 
>> silently accepting nonsense?
>
> Why is it nonsense?  The predicate gives the answer to the question
> "given these insns A and B, does A feed data that B stores in memory".
> That is a perfectly valid question to ask of any two insns.

Agreed FWIW, but for:

@@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
             if (GET_CODE (out_exp) == CLOBBER)
               continue;

-            gcc_assert (GET_CODE (out_exp) == SET);
+	    if (GET_CODE (out_exp) != SET)
+	      return false;

             if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
               return false;

how about instead changing the CLOBBER check so that we continue
when it isn't a SET?  That would allow things like UNSPECs and
USEs as well.

Thanks,
Richard

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Segher Boessenkool]

On Mon, Apr 10, 2017 at 06:38:07PM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Fri, Apr 07, 2017 at 10:39:03AM +0200, Eric Botcazou wrote:
> >> > Or we could just change "blockage" and wait for the next bug report.
> >> 
> >> That's my suggestion, yes.
> >> 
> >> > Alternatively, we can arrange for the bypass functions to not ICE.  We
> >> > can do that specific to these rs6000 pipeline descriptions, by having
> >> > our own version of store_data_bypass_p; or we can make that function
> >> > work for all insns (its definition works fine for insn pairs where
> >> > not both the producer and consumer are SETs).  That's what Kelvin's
> >> > patch does.  What is the value in ICEing here?
> >> 
> >> Telling the back-end writer that something may be wrong somewhere instead of 
> >> silently accepting nonsense?
> >
> > Why is it nonsense?  The predicate gives the answer to the question
> > "given these insns A and B, does A feed data that B stores in memory".
> > That is a perfectly valid question to ask of any two insns.
> 
> Agreed FWIW, but for:
> 
> @@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
>              if (GET_CODE (out_exp) == CLOBBER)
>                continue;
> 
> -            gcc_assert (GET_CODE (out_exp) == SET);
> +	    if (GET_CODE (out_exp) != SET)
> +	      return false;
> 
>              if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
>                return false;
> 
> how about instead changing the CLOBBER check so that we continue
> when it isn't a SET?  That would allow things like UNSPECs and
> USEs as well.

Yeah that sounds good.  Kelvin, could you try that please?


Segher

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Eric Botcazou]

[Sorry for the long delay]

> Why is it nonsense?  The predicate gives the answer to the question
> "given these insns A and B, does A feed data that B stores in memory".
> That is a perfectly valid question to ask of any two insns.

I disagree, for example it's nonsensical to send it a blockage insn.

> There are workarounds to this problem as well: mips_store_data_bypass_p,
> added in 2006.  mep_store_data_bypass_p, added in 2009 (the port has
> been removed since then, of course).

I see, no strong opinion then, but individual back-ends should be preferably 
fixed if this is easily doable instead of changing the middle-end, which may 
affect the other ~50 back-ends.

-- 
Eric Botcazou

Re: [PATCH] PR80101: Fix ICE in store_data_bypass_p

[Segher Boessenkool]

On Tue, Apr 18, 2017 at 10:01:02PM +0200, Eric Botcazou wrote:
> > There are workarounds to this problem as well: mips_store_data_bypass_p,
> > added in 2006.  mep_store_data_bypass_p, added in 2009 (the port has
> > been removed since then, of course).
> 
> I see, no strong opinion then, but individual back-ends should be preferably 
> fixed if this is easily doable instead of changing the middle-end, which may 
> affect the other ~50 back-ends.

Okay, for GCC 7 we'll change the backend.  Maybe for GCC 8 we can
revisit this.

Thanks,


Segher

[PATCH] PR80101: Fix ICE in store_data_bypass_p

[Kelvin Nilsen]

This problem reports an assertion error when certain rtl expressions
which are not eligible as producers or consumers of a store bypass
optimization are passed as arguments to the store_data_bypass_p
function.  The proposed patch returns false from store_data_bypass_p
rather than terminating with an assertion error.  False indicates that
the passed arguments are not eligible for the store bypass scheduling
optimization.

The patch has been boostrapped without regressions on
powerpc64le-unknown-linux-gnu.  Is this ok for the trunk?

gcc/ChangeLog:

2017-03-29  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	PR target/80101
	* recog.c (store_data_bypass_p): Rather than terminate with
	assertion error, return false if either function argument is not a
	single_set or a PARALLEL with SETs inside.

gcc/testsuite/ChangeLog:

2017-03-29  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	PR target/80101
	* gcc.target/powerpc/pr80101-1.c: New test.


Index: gcc/recog.c
===================================================================
--- gcc/recog.c	(revision 246469)
+++ gcc/recog.c	(working copy)
@@ -3663,9 +3663,12 @@ peephole2_optimize (void)
 
 /* Common predicates for use with define_bypass.  */
 
-/* True if the dependency between OUT_INSN and IN_INSN is on the store
-   data not the address operand(s) of the store.  IN_INSN and OUT_INSN
-   must be either a single_set or a PARALLEL with SETs inside.  */
+/* Returns true if the dependency between OUT_INSN and IN_INSN is on
+   the stored data, false if there is no dependency.  Note that a
+   consumer instruction that loads only the address (rather than the
+   value) stored by a producer instruction does not represent a
+   dependency.  If IN_INSN or OUT_INSN are not a single_set or a
+   PARALLEL with SETs inside, this function returns false.  */
 
 int
 store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn)
@@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
             if (GET_CODE (out_exp) == CLOBBER)
               continue;
 
-            gcc_assert (GET_CODE (out_exp) == SET);
+	    if (GET_CODE (out_exp) != SET)
+	      return false;
 
             if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
               return false;
@@ -3711,7 +3715,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
   else
     {
       in_pat = PATTERN (in_insn);
-      gcc_assert (GET_CODE (in_pat) == PARALLEL);
+      if (GET_CODE (in_pat) != PARALLEL)
+	return false;
 
       for (i = 0; i < XVECLEN (in_pat, 0); i++)
 	{
@@ -3720,7 +3725,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
 	  if (GET_CODE (in_exp) == CLOBBER)
 	    continue;
 
-	  gcc_assert (GET_CODE (in_exp) == SET);
+	  if (GET_CODE (in_exp) != SET)
+	    return false;
 
 	  if (!MEM_P (SET_DEST (in_exp)))
 	    return false;
@@ -3734,7 +3740,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
           else
             {
               out_pat = PATTERN (out_insn);
-              gcc_assert (GET_CODE (out_pat) == PARALLEL);
+	      if (GET_CODE (out_pat) != PARALLEL)
+		return false;
 
               for (j = 0; j < XVECLEN (out_pat, 0); j++)
                 {
@@ -3743,7 +3750,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn
                   if (GET_CODE (out_exp) == CLOBBER)
                     continue;
 
-                  gcc_assert (GET_CODE (out_exp) == SET);
+		  if (GET_CODE (out_exp) != SET)
+		    return false;
 
                   if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_exp)))
                     return false;
Index: gcc/testsuite/gcc.target/powerpc/pr80101-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80101-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80101-1.c	(working copy)
@@ -0,0 +1,22 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power6" } } */
+/* { dg-require-effective-target dfp_hw } */
+/* { dg-options "-mcpu=power6 -mno-sched-epilog -Ofast" } */
+
+/* Prior to resolving PR 80101, this test case resulted in an internal
+   compiler error.  The role of this test program is to assure that
+   dejagnu's "test for excess errors" does not find any.  */
+
+int b;
+
+void e ();
+
+int c ()
+{
+  struct
+  {
+    int a[b];
+  } d;
+  if (d.a[0])
+    e ();
+}