> On 28 Aug 2025, at 09:19, Jennifer Schmitz <[email protected]> wrote:
>
> External email: Use caution opening links or attachments
>
>
>> On 25 Aug 2025, at 15:40, Kyrylo Tkachov <[email protected]> wrote:
>>
>> Hi Jennifer,
>>
>>> On 25 Aug 2025, at 12:56, Jennifer Schmitz <[email protected]> wrote:
>>>
>>> When op2 in SVE2 saturating add intrinsics (svuqadd, svsqadd) is a zero
>>> vector and predication is _z, an ICE in vregs occurs, e.g. for
>>>
>>> svuint8_t foo (svbool_t pg, svuint8_t op1)
>>> {
>>> return svsqadd_u8_z (pg, op1, svdup_s8 (0));
>>> }
>>>
>>> The insn failed to match the pattern (aarch64-sve2.md):
>>>
>>> ;; Predicated binary operations with no reverse form, merging with zero.
>>> ;; At present we don't generate these patterns via a cond_* optab,
>>> ;; so there's no correctness requirement to handle merging with an
>>> ;; independent value.
>>> (define_insn_and_rewrite "*cond_<sve_int_op><mode>_z"
>>> [(set (match_operand:SVE_FULL_I 0 "register_operand")
>>> (unspec:SVE_FULL_I
>>> [(match_operand:<VPRED> 1 "register_operand")
>>> (unspec:SVE_FULL_I
>>> [(match_operand 5)
>>> (unspec:SVE_FULL_I
>>> [(match_operand:SVE_FULL_I 2 "register_operand")
>>> (match_operand:SVE_FULL_I 3 "register_operand")]
>>> SVE2_COND_INT_BINARY_NOREV)]
>>> UNSPEC_PRED_X)
>>> (match_operand:SVE_FULL_I 4 "aarch64_simd_imm_zero")]
>>> UNSPEC_SEL))]
>>> "TARGET_SVE2"
>>> {@ [ cons: =0 , 1 , 2 , 3 ]
>>> [ &w , Upl , 0 , w ] movprfx\t%0.<Vetype>, %1/z,
>>> %0.<Vetype>\;<sve_int_op>\t%0.<Vetype>, %1/m, %0.<Vetype>, %3.<Vetype>
>>> [ &w , Upl , w , w ] movprfx\t%0.<Vetype>, %1/z,
>>> %2.<Vetype>\;<sve_int_op>\t%0.<Vetype>, %1/m, %0.<Vetype>, %3.<Vetype>
>>> }
>>> "&& !CONSTANT_P (operands[5])"
>>> {
>>> operands[5] = CONSTM1_RTX (<VPRED>mode);
>>> }
>>> [(set_attr "movprfx" "yes")]
>>> )
>>>
>>> because operands[3] and operands[4] were both expanded into the same
>>> register
>>> operand containing a zero vector by
>>>
>>> ;; Predicated binary arithmetic with merging.
>>> (define_expand "@cond_<sve_int_op><mode>"
>>> [(set (match_operand:SVE_FULL_I 0 "register_operand")
>>> (unspec:SVE_FULL_I
>>> [(match_operand:<VPRED> 1 "register_operand")
>>> (unspec:SVE_FULL_I
>>> [(match_dup 5)
>>> (unspec:SVE_FULL_I
>>> [(match_operand:SVE_FULL_I 2 "register_operand")
>>> (match_operand:SVE_FULL_I 3 "register_operand")]
>>> SVE2_COND_INT_BINARY)]
>>> UNSPEC_PRED_X)
>>> (match_operand:SVE_FULL_I 4 "aarch64_simd_reg_or_zero")]
>>> UNSPEC_SEL))]
>>> "TARGET_SVE2"
>>> {
>>> operands[5] = CONSTM1_RTX (<MODE>mode);
>>> }
>>> )
>>>
>>> This patch fixes the ICE by changing the predicate of operands[3]
>>> in both patterns to aarch64_simd_reg_or_zero. It also adds tests
>>> adjusted from the report in PR121599.
>>>
>>> The patch was bootstrapped and tested on aarch64-linux-gnu, no regression.
>>> OK for trunk?
>>>
>>> Alex Coplan pointed out in the bugzilla ticket that this ICE goes back
>>> to GCC 10. Shall we backport?
>>>
>>> Signed-off-by: Jennifer Schmitz <[email protected]>
>>>
>>> gcc/
>>> PR target/121599
>>> * config/aarch64/aarch64-sve2.md (@cond_<sve_int_op><mode>):
>>> Set predicate for operand 3 to aarch64_simd_reg_or_zero.
>>> (*cond_<sve_int_op><mode>_z): Likewise.
>>>
>>> gcc/testsuite/
>>> PR target/121599
>>> * gcc.target/aarch64/sve2/pr121599.c: New test.
>>> ---
>>> gcc/config/aarch64/aarch64-sve2.md | 4 +--
>>> .../gcc.target/aarch64/sve2/pr121599.c | 31 +++++++++++++++++++
>>> 2 files changed, 33 insertions(+), 2 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
>>>
>>> diff --git a/gcc/config/aarch64/aarch64-sve2.md
>>> b/gcc/config/aarch64/aarch64-sve2.md
>>> index a3cbbce8b31..c6a84fa7f3d 100644
>>> --- a/gcc/config/aarch64/aarch64-sve2.md
>>> +++ b/gcc/config/aarch64/aarch64-sve2.md
>>> @@ -1021,7 +1021,7 @@
>>> [(match_dup 5)
>>> (unspec:SVE_FULL_I
>>> [(match_operand:SVE_FULL_I 2 "register_operand")
>>> - (match_operand:SVE_FULL_I 3 "register_operand")]
>>> + (match_operand:SVE_FULL_I 3 "aarch64_simd_reg_or_zero")]
>>> SVE2_COND_INT_BINARY)]
>>> UNSPEC_PRED_X)
>>> (match_operand:SVE_FULL_I 4 "aarch64_simd_reg_or_zero")]
>>> @@ -1136,7 +1136,7 @@
>>> [(match_operand 5)
>>> (unspec:SVE_FULL_I
>>> [(match_operand:SVE_FULL_I 2 "register_operand")
>>> - (match_operand:SVE_FULL_I 3 "register_operand")]
>>> + (match_operand:SVE_FULL_I 3 "aarch64_simd_reg_or_zero")]
>>> SVE2_COND_INT_BINARY_NOREV)]
>>> UNSPEC_PRED_X)
>>> (match_operand:SVE_FULL_I 4 "aarch64_simd_imm_zero")]
>>
>> I think the ICE happens because operands[4] ends up being a register which
>> the aarch64_simd_imm_zero can’t handle.
>> The pattern *cond_<sve_int_op><mode>_z is also not supposed to be handling
>> register operands, it only expresses a merge with zero.
>> Allowing a zero in operands[3] here is problematic as the output code
>> wouldn’t know how to print it if the midend used a zero vector for it.
>>
>> I think the right solution is to adjust the @cond_<sve_int_op><mode>
>> expander to only allow aarch64_simd_imm_zero for operands[4] for the
>> SVE2_COND_INT_BINARY_NOREV subset of SVE2_COND_INT_BINARY.
> Hi Kyrill,
> Thanks for reviewing the patch.
> I tried allowing only aarch64_simd_imm_zero for operands[4] for
> SVE2_COND_INT_BINARY_NOREV, but while that solved the ICE it caused all tests
> predication other than _z to fail.
> I chose instead to add a pattern similar to the existing
> *cond_<sve_int_op><mode>_3 for SVE2_COND_INT_BINARY_NOREV that handles the
> case when operands[3] and operands[4] are the same register operand.
> Thanks,
> Jennifer
Gentle ping.
Thanks,
Jennifer
>
> When op2 in SVE2 saturating add intrinsics (svuqadd, svsqadd) is a zero
> vector and predication is _z, an ICE in vregs occurs, e.g. for
>
> svuint8_t foo (svbool_t pg, svuint8_t op1)
> {
> return svsqadd_u8_z (pg, op1, svdup_s8 (0));
> }
>
> The insn failed to match the pattern (aarch64-sve2.md):
>
> ;; Predicated binary operations with no reverse form, merging with zero.
> ;; At present we don't generate these patterns via a cond_* optab,
> ;; so there's no correctness requirement to handle merging with an
> ;; independent value.
> (define_insn_and_rewrite "*cond_<sve_int_op><mode>_z"
> [(set (match_operand:SVE_FULL_I 0 "register_operand")
> (unspec:SVE_FULL_I
> [(match_operand:<VPRED> 1 "register_operand")
> (unspec:SVE_FULL_I
> [(match_operand 5)
> (unspec:SVE_FULL_I
> [(match_operand:SVE_FULL_I 2 "register_operand")
> (match_operand:SVE_FULL_I 3 "register_operand")]
> SVE2_COND_INT_BINARY_NOREV)]
> UNSPEC_PRED_X)
> (match_operand:SVE_FULL_I 4 "aarch64_simd_imm_zero")]
> UNSPEC_SEL))]
> "TARGET_SVE2"
> {@ [ cons: =0 , 1 , 2 , 3 ]
> [ &w , Upl , 0 , w ] movprfx\t%0.<Vetype>, %1/z,
> %0.<Vetype>\;<sve_int_op>\t%0.<Vetype>, %1/m, %0.<Vetype>, %3.<Vetype>
> [ &w , Upl , w , w ] movprfx\t%0.<Vetype>, %1/z,
> %2.<Vetype>\;<sve_int_op>\t%0.<Vetype>, %1/m, %0.<Vetype>, %3.<Vetype>
> }
> "&& !CONSTANT_P (operands[5])"
> {
> operands[5] = CONSTM1_RTX (<VPRED>mode);
> }
> [(set_attr "movprfx" "yes")]
> )
>
> because operands[3] and operands[4] were both expanded into the same register
> operand containing a zero vector by define_expand "@cond_<sve_int_op><mode>".
>
> This patch fixes the ICE by adding the pattern
> ;; Predicated binary operations with no reverse form, merging with
> ;; the second input.
> (define_insn_and_rewrite "*cond_<sve_int_op><mode>_3"
> for SVE2_COND_INT_BINARY_NOREV that allows operands[4] to be a register
> operand
> (equal to operands[3]). It adds a vcond_mask expression such that we end
> up with an insn matched by
> ;; Predicated binary arithmetic, merging with the first input.
> (define_insn_and_rewrite "*cond_<sve_int_op><mode>_2"
>
> The patch was bootstrapped and tested on aarch64-linux-gnu, no regression.
> OK for trunk?
>
> Alex Coplan pointed out in the bugzilla ticket that this ICE goes back
> to GCC 10. Shall we backport?
>
> Signed-off-by: Jennifer Schmitz <[email protected]>
>
> gcc/
> PR target/121599
> * config/aarch64/aarch64-sve2.md (*cond_<sve_int_op><mode>_3):
> New pattern for SVE2_COND_INT_BINARY_NOREV.
>
> gcc/testsuite/
> PR target/121599
> * gcc.target/aarch64/sve2/pr121599.c: New test.
> ---
> gcc/config/aarch64/aarch64-sve2.md | 36 +++++++++++++++++++
> .../gcc.target/aarch64/sve2/pr121599.c | 31 ++++++++++++++++
> 2 files changed, 67 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
>
> diff --git a/gcc/config/aarch64/aarch64-sve2.md
> b/gcc/config/aarch64/aarch64-sve2.md
> index a3cbbce8b31..d82dc41df3e 100644
> --- a/gcc/config/aarch64/aarch64-sve2.md
> +++ b/gcc/config/aarch64/aarch64-sve2.md
> @@ -1124,6 +1124,42 @@
> [(set_attr "movprfx" "yes")]
> )
>
> +;; Predicated binary operations with no reverse form, merging with
> +;; the second input.
> +(define_insn_and_rewrite "*cond_<sve_int_op><mode>_3"
> + [(set (match_operand:SVE_FULL_I 0 "register_operand")
> + (unspec:SVE_FULL_I
> + [(match_operand:<VPRED> 1 "register_operand")
> + (unspec:SVE_FULL_I
> + [(match_operand 5)
> + (unspec:SVE_FULL_I
> + [(match_operand:SVE_FULL_I 2 "register_operand")
> + (match_operand:SVE_FULL_I 3 "register_operand")]
> + SVE2_COND_INT_BINARY_NOREV)]
> + UNSPEC_PRED_X)
> + (match_operand:SVE_FULL_I 4 "register_operand")]
> + UNSPEC_SEL))]
> + "TARGET_SVE2
> + && rtx_equal_p (operands[3], operands[4])"
> + "#"
> + "&& 1"
> + {
> + if (reload_completed
> + && register_operand (operands[4], <MODE>mode)
> + && !rtx_equal_p (operands[0], operands[4]))
> + {
> + emit_insn (gen_vcond_mask_<mode><vpred> (operands[0], operands[2],
> + operands[4], operands[1]));
> + operands[4] = operands[2] = operands[0];
> + }
> + else if (!CONSTANT_P (operands[5]))
> + operands[5] = CONSTM1_RTX (<VPRED>mode);
> + else
> + FAIL;
> + }
> + [(set_attr "movprfx" "yes")]
> +)
> +
> ;; Predicated binary operations with no reverse form, merging with zero.
> ;; At present we don't generate these patterns via a cond_* optab,
> ;; so there's no correctness requirement to handle merging with an
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
> b/gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
> new file mode 100644
> index 00000000000..dbfa613649d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
> @@ -0,0 +1,31 @@
> +/* PR target/121599. */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { check-function-bodies "**" "" "" } } */
> +
> +#include <arm_sve.h>
> +
> +/*
> +** foo:
> +** movi d([0-9]+), #0
> +** sel z0\.b, p0, z0\.b, z\1\.b
> +** usqadd z0\.b, p0/m, z0\.b, z\1\.b
> +** ret
> +*/
> +svuint8_t foo (svbool_t pg, svuint8_t op1)
> +{
> + return svsqadd_u8_z (pg, op1, svdup_s8 (0));
> +}
> +
> +/*
> +** bar:
> +** movi d([0-9]+), #0
> +** sel z0\.b, p0, z0\.b, z\1\.b
> +** suqadd z0\.b, p0/m, z0\.b, z\1\.b
> +** ret
> +*/
> +svint8_t bar (svbool_t pg, svint8_t op1)
> +{
> + return svuqadd_n_s8_z (pg, op1, 0);
> +}
> +
> --
> 2.34.1
>>
>> Thanks,
>> Kyrill
>>
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
>>> b/gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
>>> new file mode 100644
>>> index 00000000000..cd80ef9115c
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve2/pr121599.c
>>> @@ -0,0 +1,31 @@
>>> +/* PR target/121599. */
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2" } */
>>> +/* { dg-final { check-function-bodies "**" "" "" } } */
>>> +
>>> +#include <arm_sve.h>
>>> +
>>> +/*
>>> +** foo:
>>> +** movi d([0-9]+), #0
>>> +** movprfx z0\.b, p0/z, z0\.b
>>> +** usqadd z0\.b, p0/m, z0\.b, z\1\.b
>>> +** ret
>>> +*/
>>> +svuint8_t foo (svbool_t pg, svuint8_t op1)
>>> +{
>>> + return svsqadd_u8_z (pg, op1, svdup_s8 (0));
>>> +}
>>> +
>>> +/*
>>> +** bar:
>>> +** movi d([0-9]+), #0
>>> +** movprfx z0\.b, p0/z, z0\.b
>>> +** suqadd z0\.b, p0/m, z0\.b, z\1\.b
>>> +** ret
>>> +*/
>>> +svint8_t bar (svbool_t pg, svint8_t op1)
>>> +{
>>> + return svuqadd_n_s8_z (pg, op1, 0);
>>> +}
>>> +
>>> --
>>> 2.34.1
