diff mbox

[v2] PR80101: Fix ICE in store_data_bypass_p

Message ID 02097f9b-dfc6-5508-060a-8598de98d8c6@linux.vnet.ibm.com
State New
Headers show

Commit Message

Kelvin Nilsen April 14, 2017, 10:56 p.m. UTC 
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.

Thank you for feedback and guidance received in response to my first
patch submission and the follow-on RFC post from Eric Botcazou, Segher
Boessenkool, Richard Sandiford, and Pat Haugen.  With all of your help,
I now have a much better understanding of the intended role of
store_data_bypass_p.  This new revision of the patch differs from the
original submission in the following ways:

1. I have modified the comment that describes this function to clarify
that this function is only called if it is already determined that
there exists at least one variable that is set by OUT_INSN and read by
IN_INSN. My modified comment also clarifies the function's new behavior,
as implemented with this patch. 

2. I have added comments to the body of the function to clarify some of
the rationale for the existing code and the newly inserted code,
especially where I was originally confused because I did not understand
the rationale.

3. I have added code to allow USE expressions beneath a PARALLEL node
without invalidating store data bypass (for consistency, for example,
with the implementation of single_set, and as mentioned in feedback
from Richard Sandiford).

I gather that it is extremely unlikely that in_insn would represent a
PARALLEL with multiple store operations beneath it, but this function,
as originally implemented, supports that possibility, and my changes to
the function do as well.

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

gcc/testsuite/ChangeLog:

2017-04-14  Kelvin Nilsen  <kelvin@gcc.gnu.org>

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


gcc/ChangeLog:

2017-04-14  Kelvin Nilsen  <kelvin@gcc.gnu.org>

	* recog.c (store_data_bypass_p): Rather than terminate with
	assertion error, return false if either of the function's
	arguments is not a singe_set or a PARALLEL with only SETS inside.
	Allow USE subexpressions in addition to CLOBBER subexpressions
	within a PARALLEL that represents either of the function's
	arguments.  Add and modify comments to clarify behavior.
diff mbox

Patch

Index: gcc/recog.c
===================================================================
--- gcc/recog.c	(revision 246469)
+++ gcc/recog.c	(working copy)
@@ -3663,9 +3663,14 @@  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.  */
+/* Given that there exists at least one variable that is set (produced)
+   by OUT_INSN and read (consumed) by IN_INSN, return true iff
+   IN_INSN represents one or more memory store operations and none of
+   the variables set by OUT_INSN is used by IN_INSN as the address of a
+   store operation.  If either IN_INSN or OUT_INSN does not represent
+   a "single" RTL SET expression (as loosely defined by the
+   implementation of the single_set function) or a PARALLEL with only
+   SETs, CLOBBERs, and USEs inside, this function returns false.  */
 
 int
 store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn)
@@ -3678,6 +3683,8 @@  store_data_bypass_p (rtx_insn *out_insn, rtx_insn
   in_set = single_set (in_insn);
   if (in_set)
     {
+      /* If in_set does not represent a store operation, this insn
+	 pair is not eligible for store data bypass.  */
       if (!MEM_P (SET_DEST (in_set)))
 	return false;
 
@@ -3684,6 +3691,9 @@  store_data_bypass_p (rtx_insn *out_insn, rtx_insn
       out_set = single_set (out_insn);
       if (out_set)
         {
+	  /* If the address stored by in_set is set by out_set, the
+	     dependency is on the address of the store operation, so
+	     this insn pair is not eligible for store data bypass.  */
           if (reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_set)))
             return false;
         }
@@ -3698,11 +3708,15 @@  store_data_bypass_p (rtx_insn *out_insn, rtx_insn
           {
             out_exp = XVECEXP (out_pat, 0, i);
 
-            if (GET_CODE (out_exp) == CLOBBER)
-              continue;
+	    if ((GET_CODE (out_exp) == CLOBBER) || (GET_CODE (out_exp) == USE))
+	      continue;
+            else if (GET_CODE (out_exp) != SET)
+              return false;
 
-            gcc_assert (GET_CODE (out_exp) == SET);
-
+	    /* If the address to which the in_set store operation
+	       writes is set by any of the SET subexpressions in
+	       out_insn's PARALLEL expression, this insn pair is not
+	       eligible for store data bypass.  */
             if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set)))
               return false;
           }
@@ -3711,17 +3725,21 @@  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++)
 	{
 	  in_exp = XVECEXP (in_pat, 0, i);
 
-	  if (GET_CODE (in_exp) == CLOBBER)
+	  if ((GET_CODE (in_exp) == CLOBBER) || (GET_CODE (in_exp) == USE))
 	    continue;
+	  else if (GET_CODE (in_exp) != SET)
+	    return false;
 
-	  gcc_assert (GET_CODE (in_exp) == SET);
-
+	  /* If any of the SET subexpressions belonging to in_insn's
+	     PARALLEL expression do not represent a store operation,
+	     this insn  pair is not eligible for store data bypass.  */
 	  if (!MEM_P (SET_DEST (in_exp)))
 	    return false;
 
@@ -3728,6 +3746,11 @@  store_data_bypass_p (rtx_insn *out_insn, rtx_insn
           out_set = single_set (out_insn);
           if (out_set)
             {
+	      /* If the address stored by any of the store operations
+		 represented by the PARALLEL in_insn is set by
+		 out_set, there is a dependency is on the address of
+		 the store operation, so this insn pair is not
+		 eligible for store data bypass.  */
               if (reg_mentioned_p (SET_DEST (out_set), SET_DEST (in_exp)))
                 return false;
             }
@@ -3734,17 +3757,24 @@  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++)
                 {
                   out_exp = XVECEXP (out_pat, 0, j);
 
-                  if (GET_CODE (out_exp) == CLOBBER)
-                    continue;
+		  if ((GET_CODE (out_exp) == CLOBBER)
+		      || (GET_CODE (out_exp) == USE))
+		    continue;
+                  else if (GET_CODE (out_exp) != SET)
+		    return false;
 
-                  gcc_assert (GET_CODE (out_exp) == SET);
-
+		  /* If the address stored by any of the store operations
+		     represented by the PARALLEL in_insn is set by any
+		     of the SET subexpressions in out_insn's PARALLEL
+		     expression, this insn pair is not eligible for
+		     store data bypass.  */
                   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 ();
+}