Re: [PATCH V2] [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law <jeffreyalaw@gmail.com>]

On 7/29/23 03:13, Xiao Zeng wrote:
> This patch recognizes Zicond patterns when the select pattern
> with condition eq or neq to 0 (using eq as an example), namely:
> 
> 1 rd = (rs2 == 0) ? non-imm : 0
> 2 rd = (rs2 == 0) ? non-imm : non-imm
> 3 rd = (rs2 == 0) ? reg : non-imm
> 4 rd = (rs2 == 0) ? reg : reg
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_expand_conditional_move): Recognize
>          Zicond patterns
>          * config/riscv/riscv.md: Recognize Zicond patterns through mov<mode>cc
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New test.
>          * gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New test.
>          * gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New test.
>          * gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New test.
> ---
>   gcc/config/riscv/riscv.cc                     | 144 ++++++++++++++++++
>   gcc/config/riscv/riscv.md                     |   4 +-
>   .../zicond-primitiveSemantics_return_0_imm.c  |  65 ++++++++
>   ...zicond-primitiveSemantics_return_imm_imm.c |  73 +++++++++
>   ...zicond-primitiveSemantics_return_imm_reg.c |  65 ++++++++
>   ...zicond-primitiveSemantics_return_reg_reg.c |  65 ++++++++
>   6 files changed, 414 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c
> 
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 941ea25e1f2..6ac39f63dd7 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -3516,6 +3516,150 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt)
>   							  cond, cons, alt)));
>         return true;
>       }
> +  else if (TARGET_ZICOND
> +           && (code == EQ || code == NE)
> +           && GET_MODE_CLASS (mode) == MODE_INT)
> +    {
> +      need_eq_ne_p = true;
> +      /* 0 + imm  */
> +      if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
> +          && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
A couple nits.  Rather than test the GET_CODE (object) == CONST_INT, 
instead use CONST_INT_P (object).

Rather than using const0_rtx, use CONST0_RTX (mode).  That makes it more 
general.



> +        {
> +          riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
Might as well use "true" rather than "need_eq_ne_p" here and for the 
other calls in your new code.

> +      /* imm + imm  */
> +      else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
> +               && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
So same comments on using CONST_INT_P and CONST0_RTX
> +        {
> +          riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
> +          rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> +          rtx reg = gen_reg_rtx (mode);
> +          rtx temp = GEN_INT (INTVAL (alt) - INTVAL (cons));
> +          emit_insn (gen_rtx_SET (reg, temp));
Use force_reg here rather than directly emitting the insn to initialize 
"reg".  What you're doing works when the difference is small but will 
not work when the difference does not fit into a signed 12bit value.

> +      /* imm + reg  */
> +      else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
> +               && GET_CODE (alt) == REG)
Same comments about CONST_INT_P and CONST0_RTX.  And instead of using 
GET_CODE (object) == REG, use REG_P (object).


> +        {
> +          /* Optimize for register value of 0.  */
> +          if (op0 == alt && op1 == const0_rtx)
> +            {
> +              rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> +              cons = force_reg (mode, cons);
> +              emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
> +                                                                  cons, alt)));
> +              return true;
> +            }
Isn't this only valid for NE?  Also use CONST0_RTX in that test.


> +          /* Handle the special situation of: -2048 == INTVAL (alt)
> +             to avoid failure due to an unrecognized insn. Let the costing
> +             model determine if the conditional move sequence is better
> +             than the branching sequence.  */
> +          if (-2048 == INTVAL (cons))
So instead of checking for explicit values, we have SMALL_OPERAND to do 
it for us.  Also note that for !SMALL_OPERAND we can just force the 
value into a register using "force_reg" and all the right things will 
happen.

So just add something like this to your original code:

   if (!SMALL_OPERAND (INTVAL (cons))
     cons = force_reg (mode, cons);

That will result in CONS being either a simple integer constant (when it 
is suitable for addi) or a register (all other cases).  At that point 
you can use it in an arithmetic instruction.



> +      /* imm + 0  */
> +      else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
> +               && GET_CODE (alt) == CONST_INT && alt == const0_rtx)
> +        {
> +          riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
> +          rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> +          cons = force_reg (mode, cons);
> +          emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
> +                                                              cons, alt)));
> +          return true;
> +        }
> +      /* reg + imm  */
> +      else if (GET_CODE (cons) == REG
> +               && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
> +        {
> +          /* Optimize for register value of 0.  */
> +          if (op0 == cons && op1 == const0_rtx)
> +            {
> +              rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> +              alt = force_reg (mode, alt);
> +              emit_insn (gen_rtx_SET (dest, gen_rtx_IF_THEN_ELSE (mode, cond,
> +                                                                  cons, alt)));
> +              return true;
> +            }
So in this case isn't it only valid for EQ?

> +          if (-2048 == INTVAL (alt))
Similar comment as above.  When ALT doesn't fit into a SMALL_OPERAND_P, 
just force it into a register.

> +      /* reg + reg  */
> +      else if (GET_CODE (cons) == REG && GET_CODE (alt) == REG)
> +        {
> +          /* Optimize for op0 == cons && op1 == const0_rtx.  */
> +          if (op0 == cons && op1 == const0_rtx)
> +            {
> +              rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> +              emit_insn (gen_rtx_SET (dest,
> +                                      gen_rtx_IF_THEN_ELSE (mode, cond,
> +                                                            CONST0_RTX (mode),
> +                                                            alt)));
> +              return true;
> +            }
> +          /* Optimize for op0 == alt && op1 == const0_rtx.  */
> +          if (op0 == alt && op1 == const0_rtx)
> +            {
> +              rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
> +              emit_insn (gen_rtx_SET (dest,
> +                                      gen_rtx_IF_THEN_ELSE (mode, cond, cons,
> +                                                            CONST0_RTX (mode))));
> +              return true;
> +            }
I suspect we need some testing of the code in the two sequences above. 
I'd bet one is only valid for EQ and the other for NE.

It's improving considerably.

WRT costing.  I'm still seeing some weirdness in how conditional moves 
are being costed from the if-converter.  I think your patch that set the 
cost to COSTS_N_INSNS (0) was just working around a deeper bug in the 
costing code.  I'm still trying to figure out the best way to clean that up.


jeff