[RISC-V][PR rtl-optimization/123322] Split general conditional moves into simpler...

This has gone round and round a few times... But I think we're finally at a
resolution of the regression. There's more work to do for gcc-17 though.

---

At its core this regression is caused by more accurately costing branches
during if-conversion.  The more accurate cost causes the multi-set path during
the first ifcvt pass to (profitably) convert the sequence.  However, it would
have been more profitable to have waited for things to be cleaned up and by the
2nd ifcvt pass we'd be able to convert using the condzero path which is *more*
profitable than the multi-set code (which is what happens in gcc-15).

One possible solution would be to disable the multi-set ifcvt code during its
first pass.  That was evaluated and ultimately rejected because of clear
undesirable impacts.  It may still be a good thing to do, but certainly not at
this stage of gcc-16.

Another path that was considered was recognizing the sign extension in the
if-convertable block and avoiding the multi-set path in that case on the
assumption it'd be cleaned up later and the condzero path would be used.  That
felt way too target specific and optimistic about removal of the sign
extension.

Daniel looked at if-converting these cases during phiopt; that generally looks
like a good thing to do, but had notable undesirable fallout on RISC-V and I
was significantly worried it would re-introduce sequences on x86 and aarch64
that we just fixed.  ie, solving one regression and creating another.  So that
feels like a gcc-17 evaluation.

After much head-banging it occurred to me that we could tackle this in the
target with a tiny bit of help from combine.  These conditional ops are 4
instruction sequences.  The operation, a pair of czeros and an add/ior to
select between the output of the two czeros.  Combine is pretty conservative in
when it chooses to try 4 insn sequences.

So step #1 was to loosen the restrictions on combine a little bit.  If it sees
an IF_THEN_ELSE, then it considers that "good" which in turn allows us to try 4
insn combinations a bit more often.

That exposes the potential for 4 insn combinations and since the IOR/XOR case
is a 4->2 sequence fixing those regressions is just a good define_split.   In
fact, we can use that splitter for any case where the neutral operand is zero
(IOR, XOR, PLUS, MINUS, etc).  We need a second pattern to handle reversed
operands since we're using code iterators and match_dup for the matching
operand rather than something like a matching constraint.

THat still leaves conditional shifts regressing due to the same problem.  For
shifts/rotates, the count is in QImode, so we need a different pattern to
handle those cases, but it has the same basic structure and behavior.

AND was *still* regressing though.  That would require a 4->3 split which isn't
supported by combine.  Given the issues with the other paths attempted, I
ultimately decided on a define_insn_and_split (boo!).  This shouldn't be
happening a ton like mvconst_internal, so not great, but workable.  This also
required recognizing the pattern and giving it a suitable cost (COSTS_N_INSNS
(3)).

That fixes the regressions.  We're still not generating ideal code for rv64
when the operands are 32bit quantities and the architecture provides an
instruction variant that sign extends from 32 to 64 bits (add, subtract, shifts
and rotates).  But the code we generate for gcc-16 is better than we were
generating for gcc-15, it's just not optimal.  So there's a TODO for gcc-17 as
well.

This was bootstrapped and regression tested on x86, alpha, riscv, armv7, hppa,
maybe others, but those definitely for sure.  It was also tested on the various
crosses without regressions.  Waiting on pre-commit testing to render a verdict
before going forward.

	PR rtl-optimization/123322
gcc/
	* combine.cc (try_combine): Consider an IF_THEN_ELSE "good" when
	evaluating if 4 insn combinations should be tried.
	* config/riscv/iterators.md (zero_is_neutral_op): New iterator.
	(zero_is_neutral_op_c): Likewise.
	(any_shift_rotate): Likewise.
	* config/riscv/riscv.cc (riscv_rtx_costs): Recognize the conditional
	AND RTL and cost is appropriately.
	* config/riscv/zicond.md: Add patterns to rewrite general conditional
	move sequences into simpler forms.

gcc/testsuite
	* gcc.target/riscv/pr123322.c: New test.

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 573ed716ad635818b75fbf9967996a7bbf7f882d..e0dab3d8828017b610d7dde38a8d212dcee712c7 100644 (file)
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -2615,6 +2615,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
            }
          else if (BINARY_P (src) && CONSTANT_P (XEXP (src, 1)))
            ngood++;
+         else if (GET_CODE (src) == IF_THEN_ELSE)
+           ngood++;
          else if (GET_CODE (src) == ASHIFT || GET_CODE (src) == ASHIFTRT
                   || GET_CODE (src) == LSHIFTRT)
            nshift++;

diff --git a/gcc/config/riscv/iterators.md b/gcc/config/riscv/iterators.md
index 4cda08848f6928c3f1483a89bf5c43bd7c71f045..192556e92e27dc6c8b0cc53655a6ed1e8c6c86b7 100644 (file)
--- a/gcc/config/riscv/iterators.md
+++ b/gcc/config/riscv/iterators.md
@@ -211,6 +211,13 @@
   (sign_extend "extend") (zero_extend "zero_extend")
 ])
 
+;; This code iterator captures cases where a zero value for an operand
+;; neutralizes the operation.  ie, a + 0 -> a.  That basic idea forms
+;; conditional operations.
+(define_code_iterator zero_is_neutral_op [plus minus ior xor ashift lshiftrt
+                                         ashiftrt rotatert rotate])
+(define_code_iterator zero_is_neutral_op_c [plus ior xor])
+
 ;; These code iterators allow unsigned and signed extraction to be generated
 ;; from the same template.
 (define_code_iterator any_extract [sign_extract zero_extract])
@@ -227,6 +234,8 @@
 ;; from the same template.
 (define_code_iterator any_shift [ashift ashiftrt lshiftrt])
 
+(define_code_iterator any_shift_rotate [ashift ashiftrt lshiftrt rotate rotatert])
+
 ;; This code iterator allows the three bitwise instructions to be generated
 ;; from the same template.
 (define_code_iterator any_bitwise [and ior xor])

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 32e72128f0497929ac6df72a751f1bf3f704c4b4..472c7e5f4920c2ab0ebd99eff69b7439d65d1004 100644 (file)
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4630,6 +4630,16 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
          return true;
        }
 
+      /* Conditional AND */
+      if (TARGET_ZICOND
+         && GET_CODE (XEXP (x, 0)) == IF_THEN_ELSE
+         && GET_CODE (XEXP (x, 1)) == IF_THEN_ELSE
+         && GET_CODE (XEXP (XEXP (x, 1), 1)) == AND)
+       {
+         *total = COSTS_N_INSNS (3);
+         return true;
+       }
+
       if (float_mode_p)
        *total = tune_param->fp_add[mode == DFmode];
       else

diff --git a/gcc/config/riscv/zicond.md b/gcc/config/riscv/zicond.md
index 383859b30ced99f7151f4e7cfec7eeefd74c5d27..c22559eb8d60f04205833852b10826855b2c2a7b 100644 (file)
--- a/gcc/config/riscv/zicond.md
+++ b/gcc/config/riscv/zicond.md
@@ -270,3 +270,146 @@
   [(set (match_dup 0) (ashiftrt:X (match_dup 1) (match_dup 2)))
    (set (match_dup 0) (ior:X (match_dup 0) (const_int 1)))]
   { operands[2] = GEN_INT (GET_MODE_BITSIZE (word_mode) - 1); })
+
+;; The next several splitters are mean to capture cases where if
+;; conversion was successful, but used a generalized conditional
+;; move during the first pass of if-conversion (typically because
+;; the conditional branch jumps over multiple sets).
+;;
+;; If the multiple sets would simplify into a single set, then we
+;; would often have been better off not converting during the first
+;; pass because we could use a more efficient sequence with a single
+;; czero.
+;;
+;; The basic idea here is to recognize when the two arms of the
+;; generalized conditional move have related values and where
+;; zero is a neutral operand.
+;;
+;; We conditionally zero the relevant operand, then emit the
+;; operation unconditionally.
+;;
+;; This is made more complex by the fact that 32-bit ops on rv64
+;; have an embedded sign extend.  Even worse, the mode of a shift
+;; count is QImode.  These quirks mean we have many more patterns
+;; than one might otherwise think we should.
+;;
+;; This does not handle the 32-bit ops on rv64 like addw right now.
+;; It's unclear how to do that safely since we have different modes
+;; in the two arms.
+
+;; Simple rv32 or rv64 ops where we can zero either operand to make
+;; it neutral.  Two as the the common and potentially neutral op in
+;; the 2nd if-then-else can be swapped.
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+       (plus:X (if_then_else:X
+                 (eq:X (match_operand:X 1 "register_operand") (const_int 0))
+                 (match_operand:X 2 "register_operand")
+                 (const_int 0))
+               (if_then_else:X
+                 (ne:X (match_dup 1) (const_int 0))
+                 (zero_is_neutral_op:X
+                   (match_dup 2)
+                   (match_operand:X 3 "register_operand"))
+                 (const_int 0))))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_ZICOND_LIKE || TARGET_XTHEADCONDMOV"
+  [(set (match_dup 4) (if_then_else:X (ne:X (match_dup 1) (const_int 0))
+                                     (match_dup 3)
+                                     (const_int 0)))
+   (set (match_dup 0) (zero_is_neutral_op:X (match_dup 2) (match_dup 4)))])
+
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+       (plus:X (if_then_else:X
+                 (eq:X (match_operand:X 1 "register_operand") (const_int 0))
+                 (match_operand:X 2 "register_operand")
+                 (const_int 0))
+               (if_then_else:X
+                 (ne:X (match_dup 1) (const_int 0))
+                 (zero_is_neutral_op_c:X
+                   (match_operand:X 3 "register_operand")
+                   (match_dup 2))
+                 (const_int 0))))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_ZICOND_LIKE || TARGET_XTHEADCONDMOV"
+  [(set (match_dup 4) (if_then_else:X (ne:X (match_dup 1) (const_int 0))
+                                     (match_dup 3)
+                                     (const_int 0)))
+   (set (match_dup 0) (zero_is_neutral_op_c:X (match_dup 2) (match_dup 4)))])
+
+;; This is a separate pattern because the mode on the shift count
+;; varies.  I guess we could make it modeless here and check it in
+;; the condition.  But that tends to trigger warnings from gen*.
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+       (plus:X (if_then_else:X
+                 (eq:X (match_operand:X 1 "register_operand") (const_int 0))
+                 (match_operand:X 2 "register_operand")
+                 (const_int 0))
+               (if_then_else:X
+                 (ne:X (match_dup 1) (const_int 0))
+                 (any_shift_rotate:X
+                   (match_dup 2)
+                   (match_operand:QI 3 "register_operand"))
+                 (const_int 0))))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_ZICOND_LIKE || TARGET_XTHEADCONDMOV"
+  [(set (match_dup 4) (if_then_else:X (ne:X (match_dup 1) (const_int 0))
+                                     (match_dup 3)
+                                     (const_int 0)))
+   (set (match_dup 0) (any_shift_rotate:X (match_dup 2) (subreg:QI (match_dup 4) 0)))]
+  "operands[3] = gen_lowpart (word_mode, operands[3]);")
+
+;; AND is special as the czero doesn't produce the neutral operand.  It
+;; would be a natural 4->3 split, but combine doesn't support that, so
+;; a define_insn_and_split seems to be the safest path forward to address
+;; the remaining code quality regression (match.pd approaches will likely
+;; have undesirable fallout based on what's been seen in the gcc-16 cycle).
+(define_insn_and_split "conditional_and<mode>"
+  [(set (match_operand:X 0 "register_operand" "=&r")
+       (plus:X (if_then_else:X
+                 (eq:X (match_operand:X 1 "register_operand" "r") (const_int 0))
+                 (match_operand:X 2 "register_operand" "r")
+                 (const_int 0))
+               (if_then_else:X
+                 (ne:X (match_dup 1) (const_int 0))
+                 (and:X
+                   (match_dup 2)
+                   (match_operand:X 3 "register_operand" "r"))
+                 (const_int 0))))]
+  "(TARGET_ZICOND_LIKE || TARGET_XTHEADCONDMOV) && can_create_pseudo_p ()"
+  "#"
+  "&& 1"
+  [(set (match_dup 4) (and:X (match_dup 2) (match_dup 3)))
+   (set (match_dup 5) (if_then_else:X (ne:X (match_dup 1) (const_int 0))
+                                     (match_dup 2)
+                                     (const_int 0)))
+   (set (match_dup 0) (ior:X (match_dup 5) (match_dup 4)))]
+  "operands[4] = gen_reg_rtx (word_mode);
+   operands[5] = gen_reg_rtx (word_mode);")
+
+;; Same thing, but with operand order reversed
+(define_insn_and_split "conditional_rand<mode>"
+  [(set (match_operand:X 0 "register_operand" "=&r")
+       (plus:X (if_then_else:X
+                 (eq:X (match_operand:X 1 "register_operand" "r") (const_int 0))
+                 (match_operand:X 2 "register_operand" "r")
+                 (const_int 0))
+               (if_then_else:X
+                 (ne:X (match_dup 1) (const_int 0))
+                 (and:X
+                   (match_operand:X 3 "register_operand" "r")
+                   (match_dup 2))
+                 (const_int 0))))]
+  "(TARGET_ZICOND_LIKE || TARGET_XTHEADCONDMOV) && can_create_pseudo_p ()"
+  "#"
+  "&& 1"
+  [(set (match_dup 4) (and:X (match_dup 2) (match_dup 3)))
+   (set (match_dup 5) (if_then_else:X (ne:X (match_dup 1) (const_int 0))
+                                     (match_dup 2)
+                                     (const_int 0)))
+   (set (match_dup 0) (ior:X (match_dup 5) (match_dup 4)))]
+  "operands[4] = gen_reg_rtx (word_mode);
+   operands[5] = gen_reg_rtx (word_mode);")
+

diff --git a/gcc/testsuite/gcc.target/riscv/pr123322.c b/gcc/testsuite/gcc.target/riscv/pr123322.c
new file mode 100644 (file)
index 0000000..da67596
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr123322.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32gcb_zicond -mabi=ilp32 -mbranch-cost=4" { target { rv32 } } } */
+/* { dg-options "-march=rv64gcb_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" "-Os" "-Oz" } } */
+
+#define TEST(NAME,OP) int f_int_##NAME(int a, int b, int c) { if (c) a OP b; return a; } \
+                      long f_long_##NAME(long a, long b, long c) { if (c) a OP b; return a; }
+
+TEST(xor, ^=)
+TEST(ior, |=)
+TEST(and, &=)
+TEST(plus, +=)
+TEST(minus, -=)
+TEST(rshift, >>=)
+TEST(lshift, <<=)
+
+/* AND and MULT can be handled too, but need a different neutral element that
+   we aren't handling yet.  */
+
+/* Each test should have precisely one czero.  But for rv64, the int tests
+   generate 2 for int cases where the op is widened.  */
+/* { dg-final { scan-assembler-times "czero" 14 { target rv32 } } } */
+/* { dg-final { scan-assembler-times "czero" 18 { target rv64 } } } */