diff mbox series

[to-be-committed,RISC-V,PR,target/116282] Stabilize pattern conditions

Message ID 6b889b0c-f705-4abc-befc-d1608cd8de60@gmail.com
State New
Headers show
Series [to-be-committed,RISC-V,PR,target/116282] Stabilize pattern conditions | expand

Commit Message

Jeff Law Aug. 14, 2024, 10:10 p.m. UTC
So as expected the core problem with target/116282 is that the cost of 
certain constant synthesis cases varied depending on whether or not 
we're allowed to generate new pseudos or not.

That in turn meant that in obscure cases an insn might change from 
recognizable to unrecognizable and triggers the observed failure.

So we need to keep the cost stable, at least when called from a 
pattern's condition.  So we pass another boolean down when necessary. 
I've tried to keep API fallout minimized.

Built and tested  on rv32 in my tester.  Let's see what pre-commit 
testing has to say though :-)

Note this will also require a minor change to the in-flight constant 
synthesis work.

Jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index 6872ee56022..06ff698bfe7 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -1078,7 +1078,7 @@  (define_insn_and_split ""
    && TARGET_ZBA
    && !paradoxical_subreg_p (operands[1])
    /* Only profitable if synthesis takes more than one insn.  */
-   && riscv_const_insns (operands[2]) != 1
+   && riscv_const_insns (operands[2], false) != 1
    /* We need the upper half to be zero.  */
    && (INTVAL (operands[2]) & HOST_WIDE_INT_C (0xffffffff00000000)) == 0
    /* And the the adjusted constant must either be something we can
diff --git a/gcc/config/riscv/riscv-protos.h b/gcc/config/riscv/riscv-protos.h
index 124ae2c073a..6075b824f47 100644
--- a/gcc/config/riscv/riscv-protos.h
+++ b/gcc/config/riscv/riscv-protos.h
@@ -112,7 +112,7 @@  extern bool riscv_valid_base_register_p (rtx, machine_mode, bool);
 extern enum reg_class riscv_index_reg_class ();
 extern int riscv_regno_ok_for_index_p (int);
 extern int riscv_address_insns (rtx, machine_mode, bool);
-extern int riscv_const_insns (rtx);
+extern int riscv_const_insns (rtx, bool);
 extern int riscv_split_const_insns (rtx);
 extern int riscv_load_store_insns (rtx, rtx_insn *);
 extern rtx riscv_emit_move (rtx, rtx);
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index a1b09e865ea..071d7880aeb 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -1074,11 +1074,16 @@  riscv_build_integer_1 (struct riscv_integer_op codes[RISCV_MAX_INTEGER_OPS],
 }
 
 /* Fill CODES with a sequence of rtl operations to load VALUE.
-   Return the number of operations needed.  */
+   Return the number of operations needed.
+
+   ALLOW_NEW_PSEUDOS indicates if or caller wants to allow new pseudo
+   registers or not.  This is needed for cases where the integer synthesis and
+   costing code are used in insn conditions, we can't have costing allow
+   recognition at some points and reject at others.  */
 
 static int
 riscv_build_integer (struct riscv_integer_op *codes, HOST_WIDE_INT value,
-		     machine_mode mode)
+		     machine_mode mode, bool allow_new_pseudos)
 {
   int cost = riscv_build_integer_1 (codes, value, mode);
 
@@ -1129,7 +1134,8 @@  riscv_build_integer (struct riscv_integer_op *codes, HOST_WIDE_INT value,
       int alt_cost;
 
       HOST_WIDE_INT nval = ~value;
-      alt_cost = 1 + riscv_build_integer (alt_codes, nval, mode);
+      alt_cost = 1 + riscv_build_integer (alt_codes, nval,
+					  mode, allow_new_pseudos);
       if (alt_cost < cost)
 	{
 	  alt_codes[alt_cost - 1].code = XOR;
@@ -1185,7 +1191,7 @@  riscv_build_integer (struct riscv_integer_op *codes, HOST_WIDE_INT value,
 	 using zbkb.  We may do better than that if the upper or lower half
 	 can be synthesized with a single LUI, ADDI or BSET.  Regardless the
 	 basic steps are the same.  */
-      if (cost > 3 && can_create_pseudo_p ())
+      if (cost > 3 && can_create_pseudo_p () && allow_new_pseudos)
 	{
 	  struct riscv_integer_op hi_codes[RISCV_MAX_INTEGER_OPS];
 	  struct riscv_integer_op lo_codes[RISCV_MAX_INTEGER_OPS];
@@ -1238,20 +1244,28 @@  riscv_split_integer_cost (HOST_WIDE_INT val)
   unsigned HOST_WIDE_INT hival = sext_hwi ((val - loval) >> 32, 32);
   struct riscv_integer_op codes[RISCV_MAX_INTEGER_OPS];
 
-  cost = 2 + riscv_build_integer (codes, loval, VOIDmode);
+  /* This routine isn't used by pattern conditions, so whether or
+     not to allow new pseudos can be a function of where we are in the
+     RTL pipeline.  We shouldn't need scratch pseudos for this case
+     anyway.  */
+  bool allow_new_pseudos = can_create_pseudo_p ();
+  cost = 2 + riscv_build_integer (codes, loval, VOIDmode, allow_new_pseudos);
   if (loval != hival)
-    cost += riscv_build_integer (codes, hival, VOIDmode);
+    cost += riscv_build_integer (codes, hival, VOIDmode, allow_new_pseudos);
 
   return cost;
 }
 
-/* Return the cost of constructing the integer constant VAL.  */
+/* Return the cost of constructing the integer constant VAL.  ALLOW_NEW_PSEUDOS
+   potentially restricts if riscv_build_integer is allowed to create new
+   pseudo registers.  It must be false for calls directly or indirectly from
+   conditions in patterns.  */
 
 static int
-riscv_integer_cost (HOST_WIDE_INT val)
+riscv_integer_cost (HOST_WIDE_INT val, bool allow_new_pseudos)
 {
   struct riscv_integer_op codes[RISCV_MAX_INTEGER_OPS];
-  return MIN (riscv_build_integer (codes, val, VOIDmode),
+  return MIN (riscv_build_integer (codes, val, VOIDmode, allow_new_pseudos),
 	      riscv_split_integer_cost (val));
 }
 
@@ -1528,7 +1542,9 @@  riscv_float_const_rtx_index_for_fli (rtx x)
 static bool
 riscv_legitimate_constant_p (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
 {
-  return riscv_const_insns (x) > 0;
+  /* With the post-reload usage, it seems best to just pass in FALSE
+     rather than pass ALLOW_NEW_PSEUDOS through the call chain.  */
+  return riscv_const_insns (x, false) > 0;
 }
 
 /* Implement TARGET_CANNOT_FORCE_CONST_MEM.
@@ -2076,10 +2092,14 @@  riscv_address_insns (rtx x, machine_mode mode, bool might_split_p)
 }
 
 /* Return the number of instructions needed to load constant X.
-   Return 0 if X isn't a valid constant.  */
+   Return 0 if X isn't a valid constant.
+
+   ALLOW_NEW_PSEUDOS controls whether or not we're going to be allowed
+   to create new pseduos.  It must be FALSE for any call directly or
+   indirectly from a pattern's condition.  */
 
 int
-riscv_const_insns (rtx x)
+riscv_const_insns (rtx x, bool allow_new_pseudos)
 {
   enum riscv_symbol_type symbol_type;
   rtx offset;
@@ -2096,7 +2116,7 @@  riscv_const_insns (rtx x)
 
     case CONST_INT:
       {
-	int cost = riscv_integer_cost (INTVAL (x));
+	int cost = riscv_integer_cost (INTVAL (x), allow_new_pseudos);
 	/* Force complicated constants to memory.  */
 	return cost < 4 ? cost : 0;
       }
@@ -2153,7 +2173,7 @@  riscv_const_insns (rtx x)
 		   scalar register.  Loading of a floating-point
 		   constant incurs a literal-pool access.  Allow this in
 		   order to increase vectorization possibilities.  */
-		int n = riscv_const_insns (elt);
+		int n = riscv_const_insns (elt, allow_new_pseudos);
 		if (CONST_DOUBLE_P (elt))
 		    return 1 + 4; /* vfmv.v.f + memory access.  */
 		else
@@ -2181,9 +2201,9 @@  riscv_const_insns (rtx x)
       split_const (x, &x, &offset);
       if (offset != 0)
 	{
-	  int n = riscv_const_insns (x);
+	  int n = riscv_const_insns (x, allow_new_pseudos);
 	  if (n != 0)
-	    return n + riscv_integer_cost (INTVAL (offset));
+	    return n + riscv_integer_cost (INTVAL (offset), allow_new_pseudos);
 	}
       return 0;
 
@@ -2211,8 +2231,12 @@  riscv_split_const_insns (rtx x)
 {
   unsigned int low, high;
 
-  low = riscv_const_insns (riscv_subword (x, false));
-  high = riscv_const_insns (riscv_subword (x, true));
+  /* This is not called from pattern conditions, so we can let
+     our location in the RTL pipeline control whether or not
+     new pseudos are created.  */
+  bool allow_new_pseudos = can_create_pseudo_p ();
+  low = riscv_const_insns (riscv_subword (x, false), allow_new_pseudos);
+  high = riscv_const_insns (riscv_subword (x, true), allow_new_pseudos);
   gcc_assert (low > 0 && high > 0);
   return low + high;
 }
@@ -2736,7 +2760,7 @@  riscv_move_integer (rtx temp, rtx dest, HOST_WIDE_INT value,
   mode = GET_MODE (dest);
   /* We use the original mode for the riscv_build_integer call, because HImode
      values are given special treatment.  */
-  num_ops = riscv_build_integer (codes, value, orig_mode);
+  num_ops = riscv_build_integer (codes, value, orig_mode, can_create_pseudo_p ());
 
   if (can_create_pseudo_p () && num_ops > 2 /* not a simple constant */
       && num_ops >= riscv_split_integer_cost (value))
@@ -3565,7 +3589,7 @@  riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
 
     case CONST:
       /* Non trivial CONST_INT Fall through: check if need multiple insns.  */
-      if ((cost = riscv_const_insns (x)) > 0)
+      if ((cost = riscv_const_insns (x, can_create_pseudo_p ())) > 0)
 	{
 	  /* 1. Hoist will GCSE constants only if TOTAL returned is non-zero.
 	     2. For constants loaded more than once, the approach so far has
@@ -4693,7 +4717,7 @@  riscv_emit_int_compare (enum rtx_code *code, rtx *op0, rtx *op1,
 
 	      new_rhs = rhs + (increment ? 1 : -1);
 	      new_rhs = trunc_int_for_mode (new_rhs, GET_MODE (*op0));
-	      if (riscv_integer_cost (new_rhs) < riscv_integer_cost (rhs)
+	      if (riscv_integer_cost (new_rhs, true) < riscv_integer_cost (rhs, true)
 		  && (rhs < 0) == (new_rhs < 0))
 		{
 		  *op1 = GEN_INT (new_rhs);
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 5e3ef789e42..b3bbee0a761 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -4344,10 +4344,10 @@  (define_insn_and_split ""
 		 (match_operand 3 "const_int_operand" "n")))
    (clobber (match_scratch:DI 4 "=&r"))]
   "(TARGET_64BIT
-    && riscv_const_insns (operands[3])
-    && ((riscv_const_insns (operands[3])
-	 < riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2]))))
-	|| riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2]))) == 0))"
+    && riscv_const_insns (operands[3], false)
+    && ((riscv_const_insns (operands[3], false)
+	 < riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2])), false))
+	|| riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2])), false) == 0))"
   "#"
   "&& reload_completed"
   [(set (match_dup 0) (ashift:DI (match_dup 1) (match_dup 2)))
@@ -4364,10 +4364,10 @@  (define_insn_and_split ""
 				 (match_operand 3 "const_int_operand" "n"))))
    (clobber (match_scratch:DI 4 "=&r"))]
   "(TARGET_64BIT
-    && riscv_const_insns (operands[3])
-    && ((riscv_const_insns (operands[3])
-	 < riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2]))))
-	|| riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2]))) == 0))"
+    && riscv_const_insns (operands[3], false)
+    && ((riscv_const_insns (operands[3], false)
+	 < riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2])), false))
+	|| riscv_const_insns (GEN_INT (INTVAL (operands[3]) >> INTVAL (operands[2])), false) == 0))"
   "#"
   "&& reload_completed"
   [(set (match_dup 0) (ashift:DI (match_dup 1) (match_dup 2)))
diff --git a/gcc/testsuite/gcc.target/riscv/pr116282.c b/gcc/testsuite/gcc.target/riscv/pr116282.c
new file mode 100644
index 00000000000..f86adee644d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr116282.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -march=rv64gc_zba_zbkb -mabi=lp64d" } */
+short a;
+long b;
+char c, d;
+void e(int f[][4][24][4], long g[][24][24][24]) {
+  for (unsigned h = 2;; h = 3)
+    for (long i = 0; i < 4; i = 5006368639)
+      for (int j = 0; j < 4; j = 4) {
+        for (long k = -4294967294; k < (c ?: f[0][2][6][j]); k += b)
+          a = g[k][j][0][h];
+        for (; f ? d : 0;)
+          ;
+      }
+}
+