[4/4,ifcvt] if convert x=c ? y&z : y by RISC-V Zicond like insns
Checks
Commit Message
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.
---
gcc/ifcvt.cc | 60 +++++---
.../gcc.target/riscv/zicond_ifcvt_opt.c | 134 ++++++++++++++++++
2 files changed, 176 insertions(+), 18 deletions(-)
Comments
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
@@ -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 ();
@@ -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;
+}