On Sat, Jul 29, 2023 at 04:59:00 AM Jeff Law <jeffreya...@gmail.com> 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