diff mbox

[v3,rs6000] PR80101: Fix ICE in store_data_bypass_p

Message ID 4c361398-b45c-1d15-4f2a-daa6e3ec66b2@linux.vnet.ibm.com
State New
Headers show

Commit Message

Kelvin Nilsen April 21, 2017, 4:01 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.  Since the problem surfaced with tests targeting the rs6000
architecture, the proposed patch is integrated within the rs6000 back
end.  

A new rs6000_store_data_bypass_p function has been introduced and all
calls to store_data_bypass_p from within the rs6000 back end have been
replaced with calls to rs6000_store_data_bypass_p.  This new function
scans its arguments for patterns that are known to cause assertion
errors in store_data_bypass_p and returns false if any of those
patterns are encountered.  Otherwise, rs6000_store_data_bypass_p simply
returns the result produced when passing its arguments to a call of
store_data_bypass_p.

Thank you for feedback and guidance from Eric Botcazou, Segher
Boessenkool, Richard Sandiford, and Pat Haugen which was offered in
response to my first two patch submissions and an RFC post on this
topic.  With all of your help, I now have a much better understanding
of the intended role of store_data_bypass_p.

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

gcc/testsuite/ChangeLog:

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

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


gcc/ChangeLog:

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

	PR target/80101
	* config/rs6000/power6.md: Replace store_data_bypass_p calls with
	rs6000_store_data_bypass_p in seven define_bypass directives and
	in several comments.
	* config/rs6000/rs6000-protos.h: Add prototype for
	rs6000_store_data_bypass_p function.
	* config/rs6000/rs6000.c (rs6000_store_data_bypass_p): New
	function implements slightly different (rs6000-specific) semantics
	than store_data_bypass_p, returning false rather than aborting
	with assertion error when arguments do not satisfy the
	requirements of store data bypass.
	(rs6000_adjust_cost): Replace six calls of store_data_bypass_p with
	rs6000_store_data_bypass_p.

Comments

Segher Boessenkool May 5, 2017, 3:19 p.m. UTC | #1 
Hi Kelvin,

On Fri, Apr 21, 2017 at 10:01:05AM -0600, Kelvin Nilsen wrote:
> A new rs6000_store_data_bypass_p function has been introduced and all
> calls to store_data_bypass_p from within the rs6000 back end have been
> replaced with calls to rs6000_store_data_bypass_p.  This new function
> scans its arguments for patterns that are known to cause assertion
> errors in store_data_bypass_p and returns false if any of those
> patterns are encountered.  Otherwise, rs6000_store_data_bypass_p simply
> returns the result produced when passing its arguments to a call of
> store_data_bypass_p.

> 2017-04-20  Kelvin Nilsen  <kelvin@gcc.gnu.org>
> 
> 	PR target/80101
> 	* config/rs6000/power6.md: Replace store_data_bypass_p calls with
> 	rs6000_store_data_bypass_p in seven define_bypass directives and
> 	in several comments.

Interesting that this is the only scheduling description where we use
this...  Do power8 (etc.) really not need it?

> 	* config/rs6000/rs6000-protos.h: Add prototype for
> 	rs6000_store_data_bypass_p function.
> 	* config/rs6000/rs6000.c (rs6000_store_data_bypass_p): New
> 	function implements slightly different (rs6000-specific) semantics
> 	than store_data_bypass_p, returning false rather than aborting
> 	with assertion error when arguments do not satisfy the
> 	requirements of store data bypass.
> 	(rs6000_adjust_cost): Replace six calls of store_data_bypass_p with
> 	rs6000_store_data_bypass_p.

The patch is fine for trunk.  Thanks,


Segher
diff mbox

Patch

Index: gcc/config/rs6000/power6.md
===================================================================
--- gcc/config/rs6000/power6.md	(revision 246469)
+++ gcc/config/rs6000/power6.md	(working copy)
@@ -108,7 +108,7 @@ 
                   power6-store-update-indexed,\
                   power6-fpstore,\
                   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-load-ext" 4 ; fx
   (and (eq_attr "type" "load")
@@ -128,7 +128,7 @@ 
                   power6-store-update-indexed,\
                   power6-fpstore,\
                   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-load-update" 2 ; fx
   (and (eq_attr "type" "load")
@@ -276,7 +276,7 @@ 
                   power6-store-update-indexed,\
                   power6-fpstore,\
                   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-cntlz" 2
   (and (eq_attr "type" "cntlz")
@@ -289,7 +289,7 @@ 
                   power6-store-update-indexed,\
                   power6-fpstore,\
                   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-var-rotate" 4
   (and (eq_attr "type" "shift")
@@ -355,7 +355,7 @@ 
                   power6-store-update-indexed,\
                   power6-fpstore,\
                   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-delayed-compare" 2 ; N/A
   (and (eq_attr "type" "shift")
@@ -420,7 +420,7 @@ 
                   power6-store-update-indexed,\
                   power6-fpstore,\
                   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-idiv" 44
   (and (eq_attr "type" "div")
@@ -436,7 +436,7 @@ 
 ;                  power6-store-update-indexed,\
 ;                  power6-fpstore,\
 ;                  power6-fpstore-update"
-;  "store_data_bypass_p")
+;  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-ldiv" 56
   (and (eq_attr "type" "div")
@@ -452,7 +452,7 @@ 
 ;                  power6-store-update-indexed,\
 ;                  power6-fpstore,\
 ;                  power6-fpstore-update"
-;  "store_data_bypass_p")
+;  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-mtjmpr" 2
   (and (eq_attr "type" "mtjmpr,mfjmpr")
@@ -510,7 +510,7 @@ 
 
 (define_bypass 1 "power6-fp"
                  "power6-fpstore,power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-fpcompare" 8
   (and (eq_attr "type" "fpcompare")
Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 246469)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -226,6 +226,7 @@  extern void rs6000_aix_asm_output_dwarf_table_ref
 extern void get_ppc476_thunk_name (char name[32]);
 extern bool rs6000_overloaded_builtin_p (enum rs6000_builtins);
 extern const char *rs6000_overloaded_builtin_name (enum rs6000_builtins);
+extern int rs6000_store_data_bypass_p (rtx_insn *, rtx_insn *);
 extern HOST_WIDE_INT rs6000_builtin_mask_calculate (void);
 extern void rs6000_asm_output_dwarf_pcrel (FILE *file, int size,
 					   const char *label);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 246469)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -508,6 +508,91 @@  mode_supports_pre_modify_p (machine_mode mode)
 	  != 0);
 }
 
+/* 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.
+
+   This rs6000-specific version of store_data_bypass_p checks for
+   certain conditions that result in assertion failures (and internal
+   compiler errors) in the generic store_data_bypass_p function and
+   returns false rather than calling store_data_bypass_p if one of the
+   problematic conditions is detected.  */
+
+int
+rs6000_store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn)
+{
+  rtx out_set, in_set;
+  rtx out_pat, in_pat;
+  rtx out_exp, in_exp;
+  int i, j;
+
+  in_set = single_set (in_insn);
+  if (in_set)
+    {
+      if (MEM_P (SET_DEST (in_set)))
+	{
+	  out_set = single_set (out_insn);
+	  if (!out_set)
+	    {
+	      out_pat = PATTERN (out_insn);
+	      if (GET_CODE (out_pat) == PARALLEL)
+		{
+		  for (i = 0; i < XVECLEN (out_pat, 0); i++)
+		    {
+		      out_exp = XVECEXP (out_pat, 0, i);
+		      if ((GET_CODE (out_exp) == CLOBBER)
+			  || (GET_CODE (out_exp) == USE))
+			continue;
+		      else if (GET_CODE (out_exp) != SET)
+			return false;
+		    }
+		}
+	    }
+	}
+    }
+  else
+    {
+      in_pat = PATTERN (in_insn);
+      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) || (GET_CODE (in_exp) == USE))
+	    continue;
+	  else if (GET_CODE (in_exp) != SET)
+	    return false;
+
+	  if (MEM_P (SET_DEST (in_exp)))
+	    {
+	      out_set = single_set (out_insn);
+	      if (!out_set)
+		{
+		  out_pat = PATTERN (out_insn);
+		  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)
+			  || (GET_CODE (out_exp) == USE))
+			continue;
+		      else if (GET_CODE (out_exp) != SET)
+			return false;
+		    }
+		}
+	    }
+	}
+    }
+  return store_data_bypass_p (out_insn, in_insn);
+}
+
 /* Return true if we have D-form addressing in altivec registers.  */
 static inline bool
 mode_supports_vmx_dform (machine_mode mode)
@@ -32999,7 +33084,7 @@  rs6000_adjust_cost (rtx_insn *insn, int dep_type,
                   case TYPE_LOAD:
                   case TYPE_CNTLZ:
                     {
-                      if (! store_data_bypass_p (dep_insn, insn))
+                      if (! rs6000_store_data_bypass_p (dep_insn, insn))
                         return get_attr_sign_extend (dep_insn)
                                == SIGN_EXTEND_YES ? 6 : 4;
                       break;
@@ -33006,7 +33091,7 @@  rs6000_adjust_cost (rtx_insn *insn, int dep_type,
                     }
                   case TYPE_SHIFT:
                     {
-                      if (! store_data_bypass_p (dep_insn, insn))
+                      if (! rs6000_store_data_bypass_p (dep_insn, insn))
                         return get_attr_var_shift (dep_insn) == VAR_SHIFT_YES ?
                                6 : 3;
                       break;
@@ -33017,7 +33102,7 @@  rs6000_adjust_cost (rtx_insn *insn, int dep_type,
                   case TYPE_EXTS:
                   case TYPE_INSERT:
                     {
-                      if (! store_data_bypass_p (dep_insn, insn))
+                      if (! rs6000_store_data_bypass_p (dep_insn, insn))
                         return 3;
                       break;
                     }
@@ -33026,19 +33111,19 @@  rs6000_adjust_cost (rtx_insn *insn, int dep_type,
                   case TYPE_FPSTORE:
                     {
                       if (get_attr_update (dep_insn) == UPDATE_YES
-                          && ! store_data_bypass_p (dep_insn, insn))
+                          && ! rs6000_store_data_bypass_p (dep_insn, insn))
                         return 3;
                       break;
                     }
                   case TYPE_MUL:
                     {
-                      if (! store_data_bypass_p (dep_insn, insn))
+                      if (! rs6000_store_data_bypass_p (dep_insn, insn))
                         return 17;
                       break;
                     }
                   case TYPE_DIV:
                     {
-                      if (! store_data_bypass_p (dep_insn, insn))
+                      if (! rs6000_store_data_bypass_p (dep_insn, insn))
                         return get_attr_size (dep_insn) == SIZE_32 ? 45 : 57;
                       break;
                     }
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 ();
+}