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

[Jeff Law <jeffreyalaw@gmail.com>]

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