On Sat, Jul 29, 2023 at 04:59:00 AM Jeff Law <[email protected]> wrote:
>
>
>
>On 7/19/23 04:11, Xiao Zeng wrote:
>
>> + 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)
>> + {
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + 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;
>> + }
>> + /* imm + imm */
>> + 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);
>> + alt = force_reg (mode, alt);
>> + rtx temp1 = gen_reg_rtx (mode);
>> + rtx temp2 = GEN_INT(-1 * INTVAL (cons));
>> + riscv_emit_binary(PLUS, temp1, alt, temp2);
>So in this sequence you're just computing a constant since both ALT and
>CONS are constants. It's better to just form the constant directly,
>then force that into a register because it'll make the costing more
>correct, particularly if the resulting constant needs more than one
>instruction to synthesize.
Fixed
>
>And a nit. There should always be a space between a function name and
>its argument list.
Fixed
>
>
>
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + const0_rtx, alt)));
>> + riscv_emit_binary(PLUS, dest, dest, cons);
>> + return true;
>I don't see how this can be correct from a code generation standpoint.
>You compute ALT-CONS into TEMP1 earlier. But you never use TEMP1 after
>that. I think you meant to use TEMP1 instead of ALT as the false arm if
>the IF-THEN-ELSE you constructed.
Fixed
>
>In general you should be using CONST0_RTX (mode) rather than const0_rtx.
>
Fixed
>> + }
>> + /* imm + reg */
>> + else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> + && GET_CODE (alt) == REG)
>> + {
>> + /* 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;
>> + }
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + rtx temp1 = gen_reg_rtx (mode);
>> + rtx temp2 = GEN_INT(-1 * INTVAL (cons));
>> + riscv_emit_binary(PLUS, temp1, alt, temp2);
>Here you have to be careful if CONS is -2048. You negate it resulting
>in +2048 which can't be used in an addi. This will cause the entire
>sequence to fail due to an unrecognized insn. It would be better to
>handle that scenario directly so the generated sequence is still valid.
>
>By generating recognizable code in that case we let the costing model
>determine if the conditional move sequence is better than the branching
>sequence.
Thank you for pointing out this special situation, it has been fixed
>
>
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + const0_rtx, alt)));
>I think we have the same problem with the use of ALT here rather than
>TEMP1 that we had in the previous case.
Fixed
>
>
>
>> + /* 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;
>> + }
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + rtx temp1 = gen_reg_rtx (mode);
>> + rtx temp2 = GEN_INT(-1 * INTVAL (alt));
>> + riscv_emit_binary(PLUS, temp1, cons, temp2);
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + temp1,
>> const0_rtx)));
>> + riscv_emit_binary(PLUS, dest, dest, alt);
>> + return true;
>This has basically the same issues as the imm + reg case.
Fixed
>
>
>> + }
>> + /* reg + reg */
>> + else if (GET_CODE (cons) == REG && GET_CODE (alt) == REG)
>> + {
>> + rtx reg1 = gen_reg_rtx (mode);
>> + rtx reg2 = gen_reg_rtx (mode);
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond1 = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + rtx cond2 = gen_rtx_fmt_ee (code == NE ? EQ : NE,
>> + GET_MODE (op0), op0, op1);
>> + emit_insn (gen_rtx_SET (reg2,
>> + gen_rtx_IF_THEN_ELSE (mode, cond2,
>> + const0_rtx, cons)));
>> + emit_insn (gen_rtx_SET (reg1,
>> + gen_rtx_IF_THEN_ELSE (mode, cond1,
>> + const0_rtx, alt)));
>> + riscv_emit_binary(IOR, dest, reg1, reg2);
>> + return true;
>This probably should detect the case where alt or cons is the same as
>op0. Otherwise I would expect the costing model to reject some cases
>that are actually profitable.
>
Fixed
>
>> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
>> index 6b8c2e8e268..b4147c7a79c 100644
>> --- a/gcc/config/riscv/riscv.md
>> +++ b/gcc/config/riscv/riscv.md
>> @@ -2484,7 +2484,7 @@
>> (if_then_else:GPR (match_operand 1 "comparison_operator")
>> (match_operand:GPR 2 "reg_or_0_operand")
>> (match_operand:GPR 3 "sfb_alu_operand")))
>> - "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV"
>> + "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV || TARGET_ZICOND"
>Note the predicate for operand2 will reject all constants except 0.
>
>Anyway, I'm still cleaning this up and adding the bits from Ventana
>which handle the relational conditions as well as FP conditionals.
>
>Jeff
1 Thank you for Jeff's code review comments. I have made the modifications
and submitted the V2-patch[3/5].
2 For the calculation method of cost, I hope to submit a separate patch[cost]
after the V2-patch[3/5] merged into master, which will focus on explaining
the reasons for calculating cost in the same way as in patch[4/5].
3 At the same time, I realized that for Zicond's series of patches, it would be
better to split them into separate patches and submit them to the community
code review. Therefore, I plan to submit:
V2-patch[3/5]
patch[cost]
V2-patch[4/5]
V2-patch[5/5]
I will only submit subsequent patches after the previous patch enters the
master.
4. In V2-patch[3/5], Zicond's cost calculation is not involved, therefore, all
test
cases are skipped with "- O0" and "- Os". I will remove the "- Os" constraint
from
the test case in patch[cost].
5 The address for V2-patch[3/5] is:
https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625781.html
Thanks
Xiao Zeng
