Message ID | 20231030072523.26818-5-gaofei@eswincomputing.com |
---|---|
State | New |
Headers | show |
Series | add support for conditional zero operation | expand |
On 10/30/23 01:25, Fei Gao wrote: > Conditional and, if zero > rd = (rc == 0) ? (rs1 & rs2) : rs1 > --> > and rd, rs1, rs2 > czero.eqz rtmp, rs1, rc > or rd, rd, rtmp > > Conditional and, if non-zero > rd = (rc != 0) ? (rs1 & rs2) : rs1 > --> > and rd, rs1, rs2 > czero.nez rtmp, rs1, rc > or rd, rd, rtmp > > Co-authored-by: Xiao Zeng<zengxiao@eswincomputing.com> > > gcc/ChangeLog: > > * ifcvt.cc (noce_cond_zero_binary_op_supported): add support for and > (noce_try_cond_zero_arith): adapt for and operation. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/zicond_ifcvt_opt.c: add TCs for and operation. Our internal bits also capture all these cases. The allocation is slightly different and occasionally operands are swapped in an associative operation, but the net is the same. Jeff
On 10/30/23 01:25, Fei Gao wrote: > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc > index 6e341fc4d4b..cfa9bc4b850 100644 > --- a/gcc/ifcvt.cc > +++ b/gcc/ifcvt.cc > @@ -2911,7 +2911,7 @@ noce_try_sign_mask (struct noce_if_info *if_info) > static bool > noce_cond_zero_binary_op_supported (enum rtx_code op) > { > - if (op == PLUS || op == MINUS || op == IOR || op == XOR) > + if (op == PLUS || op == MINUS || op == IOR || op == XOR || op == AND) > return true; Include ASHIFT, LSHIFTRT, ASHIFTRT, ROTATE, ROTATERT. That should pick up that critical conditional-shift-by-6 in leela. > + if (opcode == AND) > + { > + tmp > + = expand_simple_binop (mode, AND, common, z, NULL_RTX, 0, OPTAB_DIRECT); OPTAB_WIDEN here I think. > + if (!tmp) > + { > + end_sequence (); > + return FALSE; > + } > > - /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */ > - if (common && rtx_equal_p (common, if_info->x)) > - target = gen_reg_rtx (mode); > - else > - target = if_info->x; > + target = noce_emit_czero (if_info, czero_code, common, if_info->x); > + if (!target) > + { > + end_sequence (); > + return FALSE; Please try to be consistent with upper/lower case. In your prior patches you used lower case for true/false. In this patch you're using upper case. Lower case seems to be the standard in that file, so use lower case. > + } > > - target = noce_emit_czero (if_info, czero_code, z, target); > - if (!target) > - { > - end_sequence (); > - return false; > + target = expand_simple_binop (mode, IOR, tmp, target, if_info->x, 0, > + OPTAB_DIRECT); > } > + else > + { > + /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */ > + if (common && rtx_equal_p (common, if_info->x)) > + target = gen_reg_rtx (mode); > + else > + target = if_info->x; As noted before you may not be able to generate a new register when ifcvt is run after register allocation. Your code needs to handle that correctly. > + > + target = noce_emit_czero (if_info, czero_code, z, target); > + if (!target) > + { > + end_sequence (); > + return false; > + } > > - target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0, > - OPTAB_DIRECT); > + target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0, > + OPTAB_DIRECT); OPTAB_WIDEN. And the usual comments about avoiding explicit registers in the tests. I would suggest you try to handle this case as well, I don't think it's handled by your current code: long eq2 (long a, long b) { if (a == 0) return b; return 0; } There's probably also a negated version of that to be handled as well. Overall I think we can go forward with your patches after things are fixed. I'm inclined to wait until after Maciej has integrated his changes before actually committing them. While I don't expect problems, I wouldn't want Maciej to have to respin a 40+ patch series. Note that while we transition to stage3 development today, your patch was posted while we were in stage1, so you've met the deadline. We just need to get the updates done relatively soon rather than having it drag late into stage3. Jeff
On 2023-11-20 15:10 Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 10/30/23 01:25, Fei Gao wrote: > >> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc >> index 6e341fc4d4b..cfa9bc4b850 100644 >> --- a/gcc/ifcvt.cc >> +++ b/gcc/ifcvt.cc >> @@ -2911,7 +2911,7 @@ noce_try_sign_mask (struct noce_if_info *if_info) >> static bool >> noce_cond_zero_binary_op_supported (enum rtx_code op) >> { >> - if (op == PLUS || op == MINUS || op == IOR || op == XOR) >> + if (op == PLUS || op == MINUS || op == IOR || op == XOR || op == AND) >> return true; >Include ASHIFT, LSHIFTRT, ASHIFTRT, ROTATE, ROTATERT. That should pick >up that critical conditional-shift-by-6 in leela. Done. > > > > >> + if (opcode == AND) >> + { >> + tmp >> + = expand_simple_binop (mode, AND, common, z, NULL_RTX, 0, OPTAB_DIRECT); >OPTAB_WIDEN here I think. I restructured the codes to have a simple implementation. But AND is different from others operations in czero based ifcvt, I kept the ugly codes locally. I will refine it and post if current codes accepted. > > >> + if (!tmp) >> + { >> + end_sequence (); >> + return FALSE; >> + } >> >> - /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */ >> - if (common && rtx_equal_p (common, if_info->x)) >> - target = gen_reg_rtx (mode); >> - else >> - target = if_info->x; >> + target = noce_emit_czero (if_info, czero_code, common, if_info->x); >> + if (!target) >> + { >> + end_sequence (); >> + return FALSE; >Please try to be consistent with upper/lower case. In your prior >patches you used lower case for true/false. In this patch you're using >upper case. Lower case seems to be the standard in that file, so use >lower case. > >> + } >> >> - target = noce_emit_czero (if_info, czero_code, z, target); >> - if (!target) >> - { >> - end_sequence (); >> - return false; >> + target = expand_simple_binop (mode, IOR, tmp, target, if_info->x, 0, >> + OPTAB_DIRECT); >> } >> + else >> + { >> + /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */ >> + if (common && rtx_equal_p (common, if_info->x)) >> + target = gen_reg_rtx (mode); >> + else >> + target = if_info->x; >As noted before you may not be able to generate a new register when >ifcvt is run after register allocation. Your code needs to handle that >correctly. > > >> + >> + target = noce_emit_czero (if_info, czero_code, z, target); >> + if (!target) >> + { >> + end_sequence (); >> + return false; >> + } >> >> - target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0, >> - OPTAB_DIRECT); >> + target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0, >> + OPTAB_DIRECT); >OPTAB_WIDEN. > >And the usual comments about avoiding explicit registers in the tests. > > >I would suggest you try to handle this case as well, I don't think it's >handled by your current code: > >long >eq2 (long a, long b) >{ > if (a == 0) > return b; > > return 0; >} I tried both in old and new series. Zicond insns could be generated. BR, Fei > > >There's probably also a negated version of that to be handled as well. > > >Overall I think we can go forward with your patches after things are >fixed. I'm inclined to wait until after Maciej has integrated his >changes before actually committing them. While I don't expect problems, >I wouldn't want Maciej to have to respin a 40+ patch series. > >Note that while we transition to stage3 development today, your patch >was posted while we were in stage1, so you've met the deadline. We just >need to get the updates done relatively soon rather than having it drag >late into stage3. > >Jeff
diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc index 6e341fc4d4b..cfa9bc4b850 100644 --- a/gcc/ifcvt.cc +++ b/gcc/ifcvt.cc @@ -2911,7 +2911,7 @@ noce_try_sign_mask (struct noce_if_info *if_info) static bool noce_cond_zero_binary_op_supported (enum rtx_code op) { - if (op == PLUS || op == MINUS || op == IOR || op == XOR) + if (op == PLUS || op == MINUS || op == IOR || op == XOR || op == AND) return true; return false; @@ -2922,7 +2922,7 @@ noce_cond_zero_binary_op_supported (enum rtx_code op) static bool noce_try_cond_zero_arith (struct noce_if_info *if_info) { - rtx target; + rtx target, tmp; rtx_insn *seq; machine_mode mode = GET_MODE (if_info->x); rtx common = NULL_RTX; @@ -2949,8 +2949,9 @@ noce_try_cond_zero_arith (struct noce_if_info *if_info) opcode = GET_CODE (a); common = b; z = XEXP (a, 1); - /* x = c ? y+z : y, cond = !c --> x = cond ? y : y+z */ - czero_code = GET_CODE (cond); + /* x = c ? y+z : y, cond = !c --> x = cond ? y : y+z, but AND differs */ + czero_code + = (opcode == AND) ? noce_reversed_cond_code (if_info) : GET_CODE (cond); } /* check y : y+z */ else if (noce_cond_zero_binary_op_supported (GET_CODE (b)) @@ -2960,8 +2961,9 @@ noce_try_cond_zero_arith (struct noce_if_info *if_info) opcode = GET_CODE (b); common = a; z = XEXP (b, 1); - /* x = c ? y : y+z, cond = !c --> x = !cond ? y : y+z */ - czero_code = noce_reversed_cond_code (if_info); + /* x = c ? y : y+z, cond = !c --> x = !cond ? y : y+z, but AND differs */ + czero_code + = (opcode == AND) ? GET_CODE (cond) : noce_reversed_cond_code (if_info); } else return false; @@ -2970,22 +2972,44 @@ noce_try_cond_zero_arith (struct noce_if_info *if_info) return false; start_sequence (); + if (opcode == AND) + { + tmp + = expand_simple_binop (mode, AND, common, z, NULL_RTX, 0, OPTAB_DIRECT); + if (!tmp) + { + end_sequence (); + return FALSE; + } - /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */ - if (common && rtx_equal_p (common, if_info->x)) - target = gen_reg_rtx (mode); - else - target = if_info->x; + target = noce_emit_czero (if_info, czero_code, common, if_info->x); + if (!target) + { + end_sequence (); + return FALSE; + } - target = noce_emit_czero (if_info, czero_code, z, target); - if (!target) - { - end_sequence (); - return false; + target = expand_simple_binop (mode, IOR, tmp, target, if_info->x, 0, + OPTAB_DIRECT); } + else + { + /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */ + if (common && rtx_equal_p (common, if_info->x)) + target = gen_reg_rtx (mode); + else + target = if_info->x; + + target = noce_emit_czero (if_info, czero_code, z, target); + if (!target) + { + end_sequence (); + return false; + } - target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0, - OPTAB_DIRECT); + target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0, + OPTAB_DIRECT); + } if (!target) { end_sequence (); diff --git a/gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c b/gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c index 3ec01dcb135..bfff570edd7 100644 --- a/gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c +++ b/gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c @@ -506,3 +506,137 @@ long test_XOR_eqz_x_2(long x, long z, long c) x = x ^ z; return x; } + +/* +**test_AND_ceqz: +** and a2,a1,a2 +** czero\.nez a1,a1,a3 +** or a0,a1,a2 +** ret +*/ +// x = c ? y&z : y +long test_AND_ceqz(long x, long y, long z, long c) +{ + if (c) + x = y & z; + else + x = y; + return x; +} + +/* +**test_AND_ceqz_x: +** and a1,a0,a1 +** czero\.nez a0,a0,a2 +** or a0,a0,a1 +** ret +*/ +// x = c ? x&z : x +long test_AND_ceqz_x(long x, long z, long c) +{ + if (c) + x = x & z; + + return x; +} + +/* +**test_AND_nez: +** and a2,a1,a2 +** czero\.eqz a1,a1,a3 +** or a0,a1,a2 +** ret +*/ +// x = c ? y : y&z +long test_AND_nez(long x, long y, long z, long c) +{ + if (c) + x = y; + else + x = y & z; + return x; +} + +/* +**test_AND_nez_x: +** and a1,a0,a1 +** czero\.eqz a0,a0,a2 +** or a0,a0,a1 +** ret +*/ +// x = c ? x : x&z +long test_AND_nez_x(long x, long z, long c) +{ + if (c) + {} + else + x = x & z; + return x; +} + +/* +**test_AND_nez_2: +** and a2,a1,a2 +** czero\.eqz a1,a1,a3 +** or a0,a1,a2 +** ret +*/ +// x = !c ? y&z : y +long test_AND_nez_2(long x, long y, long z, long c) +{ + if (!c) + x = y & z; + else + x = y; + return x; +} + +/* +**test_AND_nez_x_2: +** and a1,a0,a1 +** czero\.eqz a0,a0,a2 +** or a0,a0,a1 +** ret +*/ +// x = !c ? x&z : x +long test_AND_nez_x_2(long x, long z, long c) +{ + if (!c) + x = x & z; + + return x; +} + +/* +**test_AND_eqz_2: +** and a2,a1,a2 +** czero\.nez a1,a1,a3 +** or a0,a1,a2 +** ret +*/ +// x = !c ? y : y&z +long test_AND_eqz_2(long x, long y, long z, long c) +{ + if (!c) + x = y; + else + x = y & z; + return x; +} + +/* +**test_AND_eqz_x_2: +** and a1,a0,a1 +** czero\.nez a0,a0,a2 +** or a0,a0,a1 +** ret +*/ +// x = !c ? x : x&z +long test_AND_eqz_x_2(long x, long z, long c) +{ + if (!c) + {} + else + x = x & z; + return x; +}