diff mbox series

[17/44] RISC-V: Avoid extraneous EQ or NE operation in cond-move expansion

Message ID alpine.DEB.2.20.2311180424040.5892@tpp.orcam.me.uk
State New
Headers show
Series RISC-V: Various if-conversion fixes and improvements | expand

Commit Message

Maciej W. Rozycki Nov. 19, 2023, 5:38 a.m. UTC
In the non-zero case there is no need for the conditional value used by 
Ventana and Zicond integer conditional operations to be specifically 1. 
Regardless we canonicalize it by producing an extraneous conditional-set 
operation, such as with the sequence below:

(insn 22 6 23 2 (set (reg:DI 141)
        (minus:DI (reg/v:DI 135 [ w ])
            (reg/v:DI 136 [ x ]))) 11 {subdi3}
     (nil))
(insn 23 22 24 2 (set (reg:DI 140)
        (ne:DI (reg:DI 141)
            (const_int 0 [0]))) 307 {*sne_zero_didi}
     (nil))
(insn 24 23 25 2 (set (reg:DI 143)
        (if_then_else:DI (eq:DI (reg:DI 140)
                (const_int 0 [0]))
            (const_int 0 [0])
            (reg:DI 13 a3 [ z ]))) 27913 {*czero.eqz.didi}
     (nil))
(insn 25 24 26 2 (set (reg:DI 142)
        (if_then_else:DI (ne:DI (reg:DI 140)
                (const_int 0 [0]))
            (const_int 0 [0])
            (reg/v:DI 137 [ y ]))) 27914 {*czero.nez.didi}
     (nil))
(insn 26 25 18 2 (set (reg/v:DI 138 [ z ])
        (ior:DI (reg:DI 142)
            (reg:DI 143))) 105 {iordi3}
     (nil))

where insn 23 can well be removed without changing the semantics of the 
sequence.  This is actually fixed up later on by combine and the insn 
does not make it to output meaning no SNEZ (or SEQZ in the reverse case) 
appears in the assembly produced, however it counts towards the cost of 
the sequence calculated by if-conversion, raising the trigger level for 
the branchless sequence to be chosen.  Arguably to emit this extraneous 
operation it can be also considered rather sloppy of our backend's.

Remove the check for operand 1 being constant 0 in the Ventana/Zicond 
case for equality comparisons then, observing that `riscv_zero_if_equal' 
called via `riscv_emit_int_compare' will canonicalize the comparison if 
required, removing the extraneous insn from output:

(insn 22 6 23 2 (set (reg:DI 142)
        (minus:DI (reg/v:DI 135 [ w ])
            (reg/v:DI 136 [ x ]))) 11 {subdi3}
     (nil))
(insn 23 22 24 2 (set (reg:DI 141)
        (if_then_else:DI (eq:DI (reg:DI 142)
                (const_int 0 [0]))
            (const_int 0 [0])
            (reg:DI 13 a3 [ z ]))) 27913 {*czero.eqz.didi}
     (nil))
(insn 24 23 25 2 (set (reg:DI 140)
        (if_then_else:DI (ne:DI (reg:DI 142)
                (const_int 0 [0]))
            (const_int 0 [0])
            (reg/v:DI 137 [ y ]))) 27914 {*czero.nez.didi}
     (nil))
(insn 25 24 18 2 (set (reg/v:DI 138 [ z ])
        (ior:DI (reg:DI 140)
            (reg:DI 141))) 105 {iordi3}
     (nil))

while keeping actual assembly produced the same.

Adjust branch costs across the test cases affected accordingly.

	gcc/
	* config/riscv/riscv.cc (riscv_expand_conditional_move): Remove 
	the check for operand 1 being constant 0 in the Ventana/Zicond
	case for equality comparisons.

	gcc/testsuite/
	* gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c: 
	Lower `-mbranch-cost=' setting.
	* gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c: 
	Likewise.
	* gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c: 
	Likewise.
	* gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c: 
	Likewise.
	* gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c: 
	Likewise.
	* gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c: 
	Likewise.
---
 gcc/config/riscv/riscv.cc                                                             |    6 +++---
 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c |    4 ++--
 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c |    4 ++--
 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c |    4 ++--
 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c |    4 ++--
 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c |    4 ++--
 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c |    4 ++--
 7 files changed, 15 insertions(+), 15 deletions(-)

gcc-riscv-expand-conditional-move-zicond-equality.diff

Comments

Jeff Law Nov. 19, 2023, 5:45 p.m. UTC | #1
On 11/18/23 22:38, Maciej W. Rozycki wrote:
> In the non-zero case there is no need for the conditional value used by
> Ventana and Zicond integer conditional operations to be specifically 1.
> Regardless we canonicalize it by producing an extraneous conditional-set
> operation, such as with the sequence below:
> 
> (insn 22 6 23 2 (set (reg:DI 141)
>          (minus:DI (reg/v:DI 135 [ w ])
>              (reg/v:DI 136 [ x ]))) 11 {subdi3}
>       (nil))
> (insn 23 22 24 2 (set (reg:DI 140)
>          (ne:DI (reg:DI 141)
>              (const_int 0 [0]))) 307 {*sne_zero_didi}
>       (nil))
> (insn 24 23 25 2 (set (reg:DI 143)
>          (if_then_else:DI (eq:DI (reg:DI 140)
>                  (const_int 0 [0]))
>              (const_int 0 [0])
>              (reg:DI 13 a3 [ z ]))) 27913 {*czero.eqz.didi}
>       (nil))
> (insn 25 24 26 2 (set (reg:DI 142)
>          (if_then_else:DI (ne:DI (reg:DI 140)
>                  (const_int 0 [0]))
>              (const_int 0 [0])
>              (reg/v:DI 137 [ y ]))) 27914 {*czero.nez.didi}
>       (nil))
> (insn 26 25 18 2 (set (reg/v:DI 138 [ z ])
>          (ior:DI (reg:DI 142)
>              (reg:DI 143))) 105 {iordi3}
>       (nil))
> 
> where insn 23 can well be removed without changing the semantics of the
> sequence.  This is actually fixed up later on by combine and the insn
> does not make it to output meaning no SNEZ (or SEQZ in the reverse case)
> appears in the assembly produced, however it counts towards the cost of
> the sequence calculated by if-conversion, raising the trigger level for
> the branchless sequence to be chosen.  Arguably to emit this extraneous
> operation it can be also considered rather sloppy of our backend's.
> 
> Remove the check for operand 1 being constant 0 in the Ventana/Zicond
> case for equality comparisons then, observing that `riscv_zero_if_equal'
> called via `riscv_emit_int_compare' will canonicalize the comparison if
> required, removing the extraneous insn from output:
> 
> (insn 22 6 23 2 (set (reg:DI 142)
>          (minus:DI (reg/v:DI 135 [ w ])
>              (reg/v:DI 136 [ x ]))) 11 {subdi3}
>       (nil))
> (insn 23 22 24 2 (set (reg:DI 141)
>          (if_then_else:DI (eq:DI (reg:DI 142)
>                  (const_int 0 [0]))
>              (const_int 0 [0])
>              (reg:DI 13 a3 [ z ]))) 27913 {*czero.eqz.didi}
>       (nil))
> (insn 24 23 25 2 (set (reg:DI 140)
>          (if_then_else:DI (ne:DI (reg:DI 142)
>                  (const_int 0 [0]))
>              (const_int 0 [0])
>              (reg/v:DI 137 [ y ]))) 27914 {*czero.nez.didi}
>       (nil))
> (insn 25 24 18 2 (set (reg/v:DI 138 [ z ])
>          (ior:DI (reg:DI 140)
>              (reg:DI 141))) 105 {iordi3}
>       (nil))
> 
> while keeping actual assembly produced the same.
> 
> Adjust branch costs across the test cases affected accordingly.
> 
> 	gcc/
> 	* config/riscv/riscv.cc (riscv_expand_conditional_move): Remove
> 	the check for operand 1 being constant 0 in the Ventana/Zicond
> 	case for equality comparisons.
> 
> 	gcc/testsuite/
> 	* gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c:
> 	Lower `-mbranch-cost=' setting.
> 	* gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c:
> 	Likewise.
> 	* gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c:
> 	Likewise.
> 	* gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c:
> 	Likewise.
> 	* gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c:
> 	Likewise.
> 	* gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c:
> 	Likewise.
OK.  Thanks for catching this!

jeff
diff mbox series

Patch

Index: gcc/gcc/config/riscv/riscv.cc
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.cc
+++ gcc/gcc/config/riscv/riscv.cc
@@ -4132,9 +4132,9 @@  riscv_expand_conditional_move (rtx dest,
 	return false;
 
       /* Canonicalize the comparison.  It must be an equality comparison
-	 against 0.  If it isn't, then emit an SCC instruction so that
-	 we can then use an equality comparison against zero.  */
-      if (!equality_operator (op, VOIDmode) || op1 != CONST0_RTX (mode))
+	 of integer operands.  If it isn't, then emit an SCC instruction
+	 so that we can then use an equality comparison against zero.  */
+      if (!equality_operator (op, VOIDmode) || !INTEGRAL_MODE_P (mode0))
 	{
 	  bool *invert_ptr = nullptr;
 	  bool invert = false;
Index: gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c
+++ gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_imm.c
@@ -1,6 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
-/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=4" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=3" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=3" { target { rv32 } } } */
 /* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
 
 long primitiveSemantics_compare_imm_return_imm_imm_00(long a, long b) {
Index: gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c
+++ gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_imm_reg.c
@@ -1,6 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
-/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=4" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=3" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=3" { target { rv32 } } } */
 /* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
 
 long primitiveSemantics_compare_imm_return_imm_reg_00(long a, long b) {
Index: gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c
+++ gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_imm_return_reg_reg.c
@@ -1,6 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=5" { target { rv64 } } } */
-/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=5" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=4" { target { rv32 } } } */
 /* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
 
 long primitiveSemantics_compare_imm_return_reg_reg_00(long a, long b, long c) {
Index: gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c
+++ gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_imm.c
@@ -1,6 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
-/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=4" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=3" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=3" { target { rv32 } } } */
 /* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
 
 long primitiveSemantics_compare_reg_return_imm_imm_00(long a, long b, long c) {
Index: gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c
+++ gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_imm_reg.c
@@ -1,6 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
-/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=4" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=3" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=3" { target { rv32 } } } */
 /* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
 
 long primitiveSemantics_compare_reg_return_imm_reg_00(long a, long b, long c) {
Index: gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c
+++ gcc/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_compare_reg_return_reg_reg.c
@@ -1,6 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=5" { target { rv64 } } } */
-/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=5" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d -mbranch-cost=4" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f -mbranch-cost=4" { target { rv32 } } } */
 /* { dg-skip-if "" { *-*-* } {"-O0" "-Og" "-Os" "-Oz"} } */
 
 long primitiveSemantics_compare_reg_return_reg_reg_00(long a, long b, long c,