On Tue, Aug 01, 2023 at 02:06:00 PM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
>
>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).
fixed
>
>Rather than using const0_rtx, use CONST0_RTX (mode). That makes it more
>general.
fixed
>
>
>
>> + {
>> + 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.
>
fixed
>> + /* 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
fixed
>> + {
>> + 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.
fixed
>
>> + /* 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?
Here is what I didn't express clearly, please see the following patterns in
zicond.md:
(define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt2"
[(set (match_operand:GPR 0 "register_operand" "=r")
(if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "r")
(match_operand:GPR 3 "register_operand" "1")))]
"TARGET_ZICOND && rtx_equal_p (operands[1], operands[3])"
"czero.nez\t%0,%2,%1"
)
Note that the EQ in the pattern becomes the opposite nez in assembly
instruction emission.
(define_insn "*czero.nez.<GPR:mode><ANYI:mode>.opt3"
[(set (match_operand:GPR 0 "register_operand" "=r")
(if_then_else:GPR (ne (match_operand:ANYI 1 "register_operand" "r")
(const_int 0))
(match_operand:GPR 2 "register_operand" "r")
(match_operand:GPR 3 "register_operand" "1")))]
"TARGET_ZICOND && rtx_equal_p (operands[1], operands[3])"
"czero.eqz\t%0,%2,%1"
)
Note that the NE in the pattern becomes the opposite eqz in assembly
instruction emission.
Therefore, EQ/NE can be used here.
>Also use CONST0_RTX in that test.
fixed
>
>> + /* 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.
First, I modified the constraints of mov<mode>cc in riscv.md:
- (match_operand:GPR 2 "reg_or_0_operand")
+ (match_operand:GPR 2 "sfb_alu_operand")
So both cons and alt will satisfy the condition: sfb_alu_operand.
The value of cons satisfies SMALL_OPERAND, but taking a negative
number on it may not satisfy SMALL_OPERAND. The only corner case here
is that is -2048 == INTVAL (cons). So in the corner case, it must be force_reg.
Then the state at this time becomes reg+reg. Rather than the case
imm+reg handled by this branch. So for the corner case I call
riscv_expand_conditional_move recursively.
Or, do you have a better suggestion for this place?
>
>
>
>> + /* 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?
As explained before, both EQ/NE are possible here.
>
>> + 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.
As explained above, they work fine.
>
>It's improving considerably.
Your code review comments push this code to be better.
>
>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.
Yes, this place deserves deep thought.
By the way, the reason why I did this *total = COSTS_N_INSNS (0) is that
the result of seq_cost in ifcvt.cc is not feasible, specifically, the result is
too
large. You can test, for a simple ADDI insn, if I remember correctly, its
result is 8. Perhaps, this result is no problem in the combine pass. As for
the Zicond's instruction, this value is larger than that of ADDI.
This causes the ifcvt pass to reject profitable optimizations at -Os.
>
>
>jeff
1 Let's clarify cost issue after v3 patch[3/5] is done.
2 v3 patch[3/5] address is:
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626015.html
Thanks
Xiao Zeng
