[PATCH] RISC-V: Also handle sign extension in branch costing

[PATCH] RISC-V: Also handle sign extension in branch costing

[Maciej W. Rozycki]

Complement commit c1e8cb3d9f94 ("RISC-V: Rework branch costing model for 
if-conversion") and also handle extraneous sign extend operations that 
are sometimes produced by `noce_try_cmove_arith' instead of zero extend 
operations, making branch costing consistent.  It is unclear what the 
condition is for the middle end to choose between the zero extend and 
sign extend operation, but the test case included uses sign extension 
with 64-bit targets, preventing if-conversion from triggering across all 
the architectural variants.

There are further anomalies revealed by the test case, specifically the 
exceedingly high branch cost of 6 required for the `-mmovcc' variant 
despite that the final branchless sequence only uses 4 instructions, the 
missed conversion at -O1 for 32-bit targets even though code is machine 
word size agnostic, and the missed conversion at -Os and -Oz for 32-bit 
Zicond targets even though the branchless sequence would be shorter than 
the branched one.  These will have to be handled separately.

	gcc/
	* config/riscv/riscv.cc (riscv_noce_conversion_profitable_p):
	Also handle sign extension.

	gcc/testsuite/
	* gcc.target/riscv/cset-sext-sfb.c: New test.
	* gcc.target/riscv/cset-sext-thead.c: New test.
	* gcc.target/riscv/cset-sext-ventana.c: New test.
	* gcc.target/riscv/cset-sext-zicond.c: New test.
	* gcc.target/riscv/cset-sext.c: New test.
---
Hi,

 This is still in regression-testing, but as a branch costing adjustment 
only I don't expect any code correctness issues, and the performance 
advantage seems very obvious as the sign extend operation applied to the 
result of a conditional set instruction is always a no-op, just as with 
the zero extension.

 Depending on how you look at it you may qualify this as a bug fix (for 
the commit referred; it's surely rare enough a case I missed in original 
testing) or a missed optimisation.  Either way it's a narrow-scoped very 
small change, almost an obviously correct one.  I'll be very happy to get 
it off my plate now, but if it has to wait for GCC 15, I'll accept the 
decision.

 OK to apply then or shall I wait?

  Maciej
---
 gcc/config/riscv/riscv.cc                          |    5 ++-
 gcc/testsuite/gcc.target/riscv/cset-sext-sfb.c     |   28 +++++++++++++++++++++
 gcc/testsuite/gcc.target/riscv/cset-sext-thead.c   |   26 +++++++++++++++++++
 gcc/testsuite/gcc.target/riscv/cset-sext-ventana.c |   26 +++++++++++++++++++
 gcc/testsuite/gcc.target/riscv/cset-sext-zicond.c  |   26 +++++++++++++++++++
 gcc/testsuite/gcc.target/riscv/cset-sext.c         |   27 ++++++++++++++++++++
 6 files changed, 136 insertions(+), 2 deletions(-)

gcc-riscv-noce-conversion-profitable-p-sign-extend.diff
Index: gcc/gcc/config/riscv/riscv.cc
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.cc
+++ gcc/gcc/config/riscv/riscv.cc
@@ -3555,7 +3555,7 @@ riscv_noce_conversion_profitable_p (rtx_
      this redundant zero extend operation counts towards the cost of
      the replacement sequence.  Compensate for that by incrementing the
      cost of the original sequence as well as the maximum sequence cost
-     accordingly.  */
+     accordingly.  Likewise for sign extension.  */
   rtx last_dest = NULL_RTX;
   for (rtx_insn *insn = seq; insn; insn = NEXT_INSN (insn))
     {
@@ -3567,8 +3567,9 @@ riscv_noce_conversion_profitable_p (rtx_
 	  && GET_CODE (x) == SET)
 	{
 	  rtx src = SET_SRC (x);
+	  enum rtx_code code = GET_CODE (src);
 	  if (last_dest != NULL_RTX
-	      && GET_CODE (src) == ZERO_EXTEND
+	      && (code == SIGN_EXTEND || code == ZERO_EXTEND)
 	      && REG_P (XEXP (src, 0))
 	      && REGNO (XEXP (src, 0)) == REGNO (last_dest))
 	    {
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-sfb.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-sfb.c
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv32gc -mtune=sifive-7-series -mbranch-cost=1 -fdump-rtl-ce1" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc -mtune=sifive-7-series -mbranch-cost=1 -fdump-rtl-ce1" { target { rv64 } } } */
+
+int
+foo (long a, long b)
+{
+  if (!b)
+    return 0;
+  else if (a)
+    return 1;
+  else
+    return 0;
+}
+
+/* Expect short forward branch assembly like:
+
+	snez	a0,a0
+	bne	a1,zero,1f	# movcc
+	mv	a0,zero
+1:
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\sbne\\s\[^\\s\]+\\s# movcc\\s" 1 { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-not "\\sbeq\\s" { xfail { rv32 && { any-opts "-O1" } } } } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-thead.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-thead.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target rv64 } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv64gc_xtheadcondmov -mtune=thead-c906 -mbranch-cost=1 -fdump-rtl-ce1" } */
+
+int
+foo (long a, long b)
+{
+  if (!b)
+    return 0;
+  else if (a)
+    return 1;
+  else
+    return 0;
+}
+
+/* Expect branchless assembly like:
+
+	snez	a0,a0
+	th.mveqz	a0,zero,a1
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\s(?:th\\.mveqz|th\\.mvnez)\\s" 1 } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-ventana.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-ventana.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target rv64 } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv64gc_xventanacondops -mtune=rocket -mbranch-cost=3 -fdump-rtl-ce1" } */
+
+int
+foo (long a, long b)
+{
+  if (!b)
+    return 0;
+  else if (a)
+    return 1;
+  else
+    return 0;
+}
+
+/* Expect branchless assembly like:
+
+	snez	a0,a0
+	vt.maskc	a0,a0,a1
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\svt\\.maskc\\s" 1 } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-zicond.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-zicond.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv64gc_zicond -mtune=rocket -mbranch-cost=3 -fdump-rtl-ce1" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mtune=rocket -mbranch-cost=3 -fdump-rtl-ce1" { target { rv32 } } } */
+
+int
+foo (long a, long b)
+{
+  if (!b)
+    return 0;
+  else if (a)
+    return 1;
+  else
+    return 0;
+}
+
+/* Expect branchless assembly like:
+
+	snez	a0,a0
+	czero.eqz	a0,a0,a1
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" { xfail { rv32 && { any-opts "-O1" "-Os" "-Oz" } } } } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\sczero\\.eqz\\s" 1 { xfail { rv32 && { any-opts "-O1" "-Os" "-Oz" } } } } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" { xfail { rv32 && { any-opts "-O1" "-Os" "-Oz" } } } } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-Os" "-Oz" } } */
+/* { dg-options "-march=rv32gc -mtune=sifive-5-series -mbranch-cost=6 -mmovcc -fdump-rtl-ce1" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc -mtune=sifive-5-series -mbranch-cost=6 -mmovcc -fdump-rtl-ce1" { target { rv64 } } } */
+
+int
+foo (long a, long b)
+{
+  if (!b)
+    return 0;
+  else if (a)
+    return 1;
+  else
+    return 0;
+}
+
+/* Expect branchless assembly like:
+
+	snez	a1,a1
+	neg	a1,a1
+	snez	a0,a0
+	and	a0,a1,a0
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 2 { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" { xfail { rv32 && { any-opts "-O1" } } } } } */

Re: [PATCH] RISC-V: Also handle sign extension in branch costing

[Jeff Law]

On 1/7/24 17:06, Maciej W. Rozycki wrote:
> Complement commit c1e8cb3d9f94 ("RISC-V: Rework branch costing model for
> if-conversion") and also handle extraneous sign extend operations that
> are sometimes produced by `noce_try_cmove_arith' instead of zero extend
> operations, making branch costing consistent.  It is unclear what the
> condition is for the middle end to choose between the zero extend and
> sign extend operation, but the test case included uses sign extension
> with 64-bit targets, preventing if-conversion from triggering across all
> the architectural variants.
> 
> There are further anomalies revealed by the test case, specifically the
> exceedingly high branch cost of 6 required for the `-mmovcc' variant
> despite that the final branchless sequence only uses 4 instructions, the
> missed conversion at -O1 for 32-bit targets even though code is machine
> word size agnostic, and the missed conversion at -Os and -Oz for 32-bit
> Zicond targets even though the branchless sequence would be shorter than
> the branched one.  These will have to be handled separately.
> 
> 	gcc/
> 	* config/riscv/riscv.cc (riscv_noce_conversion_profitable_p):
> 	Also handle sign extension.
> 
> 	gcc/testsuite/
> 	* gcc.target/riscv/cset-sext-sfb.c: New test.
> 	* gcc.target/riscv/cset-sext-thead.c: New test.
> 	* gcc.target/riscv/cset-sext-ventana.c: New test.
> 	* gcc.target/riscv/cset-sext-zicond.c: New test.
> 	* gcc.target/riscv/cset-sext.c: New test.
> ---
> Hi,
> 
>   This is still in regression-testing, but as a branch costing adjustment
> only I don't expect any code correctness issues, and the performance
> advantage seems very obvious as the sign extend operation applied to the
> result of a conditional set instruction is always a no-op, just as with
> the zero extension.
> 
>   Depending on how you look at it you may qualify this as a bug fix (for
> the commit referred; it's surely rare enough a case I missed in original
> testing) or a missed optimisation.  Either way it's a narrow-scoped very
> small change, almost an obviously correct one.  I'll be very happy to get
> it off my plate now, but if it has to wait for GCC 15, I'll accept the
> decision.
> 
>   OK to apply then or shall I wait?
OK to apply.

jeff

Re: [PATCH] RISC-V: Also handle sign extension in branch costing

[Maciej W. Rozycki]

On Tue, 9 Jan 2024, Jeff Law wrote:

> >   Depending on how you look at it you may qualify this as a bug fix (for
> > the commit referred; it's surely rare enough a case I missed in original
> > testing) or a missed optimisation.  Either way it's a narrow-scoped very
> > small change, almost an obviously correct one.  I'll be very happy to get
> > it off my plate now, but if it has to wait for GCC 15, I'll accept the
> > decision.
> > 
> >   OK to apply then or shall I wait?
> OK to apply.

 Thank you for your review, I have now pushed this change.

  Maciej