Hi Kyrill, Thanks for the review!
> On 13 Jul 2026, at 8:54 PM, Kyrylo Tkachov <[email protected]> wrote: > > Hi Soumya, > >> On 30 Jun 2026, at 10:24, Soumya AR <[email protected]> wrote: >> >> Ping. >> >> Also CC'ing Andrea, could you please take a look if possible? >> >> Thanks, >> Soumya >> >> On on Tue, 16 Jun 2026 10:54:37 +0000, Soumya AR <[email protected]> wrote: >>> Ping. >>> >>> Thanks, >>> Soumya >>> >>>> On 8 Jun 2026, at 11:23 PM, Soumya AR <[email protected]> wrote: >>>> >>>> Ping. >>>> >>>> Thanks, >>>> Soumya >>>> >>>>> On 26 May 2026, at 2:24 PM, Soumya AR <[email protected]> wrote: >>>>> >>>>> Hi, >>>>> >>>>> Reviving this patch because it completely slipped my mind and has been >>>>> sitting >>>>> in limbo ever since. >>>>> >>>>> I'll try to summarize the discussion for some context: >>>>> >>>>> This patch teaches expand_divmod to dispatch through the sdiv_pow2 optab >>>>> for >>>>> vector pow2 divisions, so that > > > > >>>>> >>>>> svint32_t bar (svint32_t x) { return x / 4; } >>>>> >>>>> emits a single ASRD instead of a vector SDIV. ASRD on SVE performs signed >>>>> division by an arithmetic shift right that rounds toward zero. >>>>> >>>>> The autovectorizer already uses this optab via vect_recog_divmod_pattern; >>>>> this >>>>> extends the same mechanism to the non-vectorizer path. >>>>> >>>>> There are a couple of backend approaches we could've taken: >>>>> >>>>> - make expand_divmod emit the shift sequence for vector pow2 divisions >>>>> (similar >>>>> to how it handles scalars) and and rely on combine to recognize it back >>>>> into >>>>> ASRD. >>>>> >>>>> - extend the SVE SDIV pattern to accept immediates and emit ASRD for pow2. >>>>> >>>>> But those feel less clean given we have a dedicated optab (and that it >>>>> would be >>>>> neater to be consistent with the autovectorizer approach). >>>>> >>>>> Note that this also applies to Adv. SIMD vectors on SVE targets, since the >>>>> AArch64 backend exposes sdiv_pow2 for Adv. SIMD modes. >>>>> >>>>> For example, both: >>>>> >>>>> int32x4_t >>>>> foo (int32x4_t x) >>>>> { >>>>> return x / 4; >>>>> } >>>>> >>>>> and >>>>> >>>>> svint32_t >>>>> bar (svint32_t x) >>>>> { >>>>> return x / 4; >>>>> } >>>>> >>>>> currently generate DIV, but can be done using ASRD. >>>>> >>>>> The patch was bootstrapped and regtested on aarch64-linux-gnu, no >>>>> regression. >>>>> OK for trunk? >>>>> >>>>> Signed-off-by: Soumya AR <[email protected]> >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> * expmed.cc (expand_divmod): Expand to sdiv_pow2 optab for vectors. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * gcc.target/aarch64/sve/sve-asrd-2.c: New test. >>>>> --- >>>>> gcc/expmed.cc | 21 ++++++ >>>>> .../gcc.target/aarch64/sve/sve-asrd-2.c | 74 +++++++++++++++++++ >>>>> 2 files changed, 95 insertions(+) >>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/sve-asrd-2.c >>>>> >>>>> diff --git a/gcc/expmed.cc b/gcc/expmed.cc >>>>> index fe39506b6bd..ef12420689f 100644 >>>>> --- a/gcc/expmed.cc >>>>> +++ b/gcc/expmed.cc >>>>> @@ -4487,6 +4487,27 @@ expand_divmod (int rem_flag, enum tree_code code, >>>>> machine_mode mode, >>>>> { >>>>> case TRUNC_MOD_EXPR: >>>>> case TRUNC_DIV_EXPR: >>>>> + if (CONST_VECTOR_P (op1) >>>>> + && optab_handler (sdiv_pow2_optab, mode) != CODE_FOR_nothing) >>>>> + { > > I think it is wrong to use the sdiv_pow2_optab when unsignedp is passed in > this function, otherwise inputs like: > { 0x80000000, 0xffffffff, 16, 7 } / 8 > > will give wrong results My bad, added the check for this. > >>>>> + rtx scalar_op1; >>>>> + if (const_vec_duplicate_p (op1, &scalar_op1) >>>>> + && CONST_INT_P (scalar_op1)) >>>>> + { >>>>> + HOST_WIDE_INT d = INTVAL (scalar_op1); >>>>> + if (d > 0 && pow2p_hwi (d)) >>>>> + { >>>>> + rtx shift_amount >>>>> + = gen_const_vec_duplicate (mode, >>>>> + GEN_INT (floor_log2 (d))); >>>>> + rtx result >>>>> + = expand_binop (mode, sdiv_pow2_optab, op0, shift_amount, >>>>> + target, unsignedp, methods); >>>>> + if (result) >>>>> + return result; > > > For TRUNC_MOD_EXPR result is the quotient, I think this new path should only > be used for !rem_flag. > Alternatively, assign the result to quotient and continue through the common > remainder handling. > I experimented with using the common handling for this. Directly assigning the result to quotient and falling through doesn't work since the next check is op1_is_constant (false for a CONST_VECTOR). Control therefore falls through to fail1:, where delete_insns_since removes the emitted ASRD. We could just bypass this, but that might be moot, since vector pow2 modulo is actually processed earlier by expand_vector_divmod. However, this only works for NEON cases, for SVE modulo, we get the same ICE mentioned here: gcc.gnu.org/bugzilla/show_bug.cgi?id=118378 For now, I have just added !rem_flag. >>>>> + } >>>>> + } >>>>> + } >>>>> if (op1_is_constant) >>>>> { >>>>> scalar_int_mode int_mode = as_a <scalar_int_mode> (compute_mode); >>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/sve-asrd-2.c >>>>> b/gcc/testsuite/gcc.target/aarch64/sve/sve-asrd-2.c >>>>> new file mode 100644 >>>>> index 00000000000..a5163421a69 >>>>> --- /dev/null >>>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/sve-asrd-2.c >>>>> @@ -0,0 +1,74 @@ >>>>> +/* { dg-do compile } */ >>>>> +/* { dg-additional-options "-O3" } */ >>>>> +/* { dg-final { check-function-bodies "**" "" "" } } */ >>>>> + >>>>> +#include <arm_sve.h> >>>>> + >>>>> +#define FOO(type) \ >>>>> +type foo_##type(type x) { \ >>>>> + return x / 8; \ >>>>> +} >>>>> + >>>>> +/* >>>>> +** foo_int32x4_t: >>>>> +** ptrue (p[0-7]).b, vl16 >>>>> +** asrd z[0-9]+\.s, \1/m, z[0-9]+\.s, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(int32x4_t) >>>>> + >>>>> +/* >>>>> +** foo_int64x2_t: >>>>> +** ptrue (p[0-7]).b, vl16 >>>>> +** asrd z[0-9]+\.d, \1/m, z[0-9]+\.d, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(int64x2_t) >>>>> + >>>>> +/* >>>>> +** foo_svint32_t: >>>>> +** ptrue (p[0-7]).b, all >>>>> +** asrd z[0-9]+\.s, \1/m, z[0-9]+\.s, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(svint32_t) >>>>> + >>>>> +/* >>>>> +** foo_svint64_t: >>>>> +** ptrue (p[0-7]).b, all >>>>> +** asrd z[0-9]+\.d, \1/m, z[0-9]+\.d, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(svint64_t) >>>>> + >>>>> +/* >>>>> +** foo_uint32x4_t: >>>>> +** ptrue (p[0-7]).b, vl16 >>>>> +** asrd z[0-9]+\.s, \1/m, z[0-9]+\.s, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(uint32x4_t) > > As mentioned above, I don’t think using ASRD this is correct for unsigned > types so these tests would need to be removed. Done. Thanks, Soumya > > Thanks, > Kyrill > >>>>> + >>>>> +/* >>>>> +** foo_uint64x2_t: >>>>> +** ptrue (p[0-7]).b, vl16 >>>>> +** asrd z[0-9]+\.d, \1/m, z[0-9]+\.d, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(uint64x2_t) >>>>> + >>>>> +/* >>>>> +** foo_svuint32_t: >>>>> +** ptrue (p[0-7]).b, all >>>>> +** asrd z[0-9]+\.s, \1/m, z[0-9]+\.s, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(svuint32_t) >>>>> + >>>>> +/* >>>>> +** foo_svuint64_t: >>>>> +** ptrue (p[0-7]).b, all >>>>> +** asrd z[0-9]+\.d, \1/m, z[0-9]+\.d, #3 >>>>> +** ret >>>>> +*/ >>>>> +FOO(svuint64_t) >>>>> -- >>>>> 2.43.0 >>>>> >>>>> >>>>>> On 22 Oct 2025, at 7:21 PM, Tamar Christina <[email protected]> >>>>>> wrote: >>>>>> >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Richard Biener <[email protected]> >>>>>>> Sent: 22 October 2025 14:30 >>>>>>> To: Tamar Christina <[email protected]> >>>>>>> Cc: Soumya AR <[email protected]>; [email protected]; Kyrylo >>>>>>> Tkachov <[email protected]> >>>>>>> Subject: RE: [PATCH] aarch64: Use SVE ASRD with vector division using >>>>>>> division operator >>>>>>> >>>>>>> On Wed, 22 Oct 2025, Tamar Christina wrote: >>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Richard Biener <[email protected]> >>>>>>>>> Sent: 22 October 2025 14:05 >>>>>>>>> To: Tamar Christina <[email protected]> >>>>>>>>> Cc: Soumya AR <[email protected]>; [email protected]; Kyrylo >>>>>>>>> Tkachov <[email protected]> >>>>>>>>> Subject: RE: [PATCH] aarch64: Use SVE ASRD with vector division using >>>>>>>>> division operator >>>>>>>>> >>>>>>>>> On Wed, 22 Oct 2025, Tamar Christina wrote: >>>>>>>>> >>>>>>>>>> Hi Soumya, >>>>>>>>>> >>>>>>>>>>> -----Original Message----- >>>>>>>>>>> From: Soumya AR <[email protected]> >>>>>>>>>>> Sent: 13 October 2025 13:42 >>>>>>>>>>> To: Tamar Christina <[email protected]> >>>>>>>>>>> Cc: [email protected]; Kyrylo Tkachov <[email protected]>; >>>>>>>>>>> Richard Biener <[email protected]> >>>>>>>>>>> Subject: Re: [PATCH] aarch64: Use SVE ASRD with vector division >>>>>>>>>>> using >>>>>>>>>>> division operator >>>>>>>>>>> Importance: High >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> On 13 Oct 2025, at 1:48 PM, Tamar Christina >>>>>>>>> <[email protected]> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> External email: Use caution opening links or attachments >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi Soumya, >>>>>>>>>>>> >>>>>>>>>>>> The 10/13/2025 07:05, Soumya AR wrote: >>>>>>>>>>>>> The ASRD instruction on SVE performs an arithmetic shift right by >>>>>>>>>>>>> an >>>>>>>>>>> immediate >>>>>>>>>>>>> for divide. This patch enables ASRD when dividing vectors using >>>>>>>>>>>>> the >>>>>>> GNU >>>>>>>>> C >>>>>>>>>>>>> division operator. >>>>>>>>>>>>> >>>>>>>>>>>>> For example: >>>>>>>>>>>>> >>>>>>>>>>>>> int32x4_t >>>>>>>>>>>>> foo (int32x4_t x) >>>>>>>>>>>>> { >>>>>>>>>>>>> return x / 4; >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> svint32_t >>>>>>>>>>>>> bar (svint32_t x) >>>>>>>>>>>>> { >>>>>>>>>>>>> return x / 4; >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> currently generates a DIV, but can be done using ASRD. >>>>>>>>>>>>> >>>>>>>>>>>>> The patch was bootstrapped and regtested on aarch64-linux-gnu, no >>>>>>>>>>> regression. >>>>>>>>>>>>> OK for GCC16? >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Soumya AR <[email protected]> >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> * expmed.cc (expand_divmod): Expand to sdiv_pow2 optab for >>>>>>>>> vectors. >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> * gcc.target/aarch64/sve/sve-asrd-2.c: New test. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Hi Tamar, >>>>>>>>>>> >>>>>>>>>>> Thanks for the quick review! >>>>>>>>>>> >>>>>>>>>>>> The test is OK, but you'll need a middle-end maintainer for the >>>>>>> expmed.cc >>>>>>>>>>> changes. >>>>>>>>>>> >>>>>>>>>>> My bad, CC'ing Richi here as well. >>>>>>>>>>> >>>>>>>>>>>> But that said... >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/gcc/expmed.cc b/gcc/expmed.cc >>>>>>>>>>>> index df09cbccd08..a35aa229d44 100644 >>>>>>>>>>>> --- a/gcc/expmed.cc >>>>>>>>>>>> +++ b/gcc/expmed.cc >>>>>>>>>>>> @@ -4465,6 +4465,24 @@ expand_divmod (int rem_flag, enum >>>>>>>>> tree_code >>>>>>>>>>> code, machine_mode mode, >>>>>>>>>>>> { >>>>>>>>>>>> case TRUNC_MOD_EXPR: >>>>>>>>>>>> case TRUNC_DIV_EXPR: >>>>>>>>>>>> + if (CONST_VECTOR_P (op1) >>>>>>>>>>>> + && optab_handler (sdiv_pow2_optab, mode) != >>>>>>>>> CODE_FOR_nothing) >>>>>>>>>>>> + { >>>>>>>>>>>> + rtx scalar_op1 = unwrap_const_vec_duplicate (op1); >>>>>>>>>>>> + if (scalar_op1 != NULL_RTX && CONST_INT_P (scalar_op1)) >>>>>>>>>>>> >>>>>>>>>>>> You can simplify this using const_vec_duplicate_p. >>>>>>>>>>>> >>>>>>>>>>>> + { >>>>>>>>>>>> + HOST_WIDE_INT d = INTVAL (scalar_op1); >>>>>>>>>>>> + if (d > 0 && pow2p_hwi (d)) >>>>>>>>>>>> + { >>>>>>>>>>>> + rtx shift_amount >>>>>>>>>>>> + = gen_const_vec_duplicate (mode, >>>>>>>>>>>> + GEN_INT >>>>>>>>>>>> (floor_log2 (d))); >>>>>>>>>>>> + return expand_binop (mode, sdiv_pow2_optab, >>>>>>>>>>>> op0, >>>>>>>>>>>> + shift_amount, target, >>>>>>>>>>>> unsignedp, >>>>>>>>>>>> + methods); >>>>>>>>>>>> >>>>>>>>>>>> This expansion can fail, when e.g. the shift amount is larger than >>>>>>>>>>>> the >>>>>>>>> bitsize >>>>>>>>>>> of the element, >>>>>>>>>>>> so you should check the result of it before exiting from >>>>>>> expand_divmod. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Makes sense, attaching an updated patch with both changes. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> The new patch looks OK to me (it uses the old interface but that's >>>>>>>>>> what >>>>>>> most >>>>>>>>> of >>>>>>>>>> expmed uses). >>>>>>>>>> >>>>>>>>>> I think your cover letter needs to mention that we can't do this in >>>>>>>>>> the >>>>>>>>> backend because >>>>>>>>>> Adv. SIMD lacks a division instruction and that the decomposition is >>>>>>> harder >>>>>>>>> to match >>>>>>>>>> correctly in all cases. >>>>>>>>>> >>>>>>>>>> That said you still need a middle-end maintainer approval. >>>>>>>>> >>>>>>>>> So the backend advertises a vector division? Othewise I would have >>>>>>>>> expected vector lowering to lower this (if it does that at all, for >>>>>>>>> VLA vectors?!). >>>>>>>>> >>>>>>>>> So this is an alternative better expansion - but there should already >>>>>>>>> be generic code to deal with divisions by a power of two, why does >>>>>>>>> that not trigger? And what's this sdiv_pow2 thing again? >>>>>>>> >>>>>>>> Yes, expand intercepts in expand_divmod division by a power of 2 and >>>>>>> decomposes >>>>>>>> It to shifts. We could re-recognize the instruction in combine of >>>>>>>> course, but >>>>>>> we already >>>>>>>> have an optab the vectorizer would have used in this case >>>>>>>> [us]div_pow2, see >>>>>>>> vect_recog_divmod_pattern. So this is just teaching expand about the >>>>>>> special division of >>>>>>>> a power of 2 optab that the vectorizer uses for normal vector >>>>>>>> operations. >>>>>>> >>>>>>> I can't remember the difference of div_pow2 and shifts, but it feels >>>>>>> like >>>>>>> this expand-time improvement should be done by expand_divmod then? >>>>>> >>>>>> The patch does extend expand_divmod though, but does through an existing >>>>>> optab. >>>>>> >>>>>> The issue is that signedness matters, >>>>>> this rewrite to asrd is only possible for signed values as it's an >>>>>> arithmetic shift. >>>>>> >>>>>> For unsigned values we want ushr. >>>>>> >>>>>>> 'asrd' is described as arithmetic shift right for divide by immediate >>>>>>> whatever the exact difference from 'asr' is ... the docs say sth >>>>>>> about round towards zero, but then we're using arithmetic right shifts >>>>>>> for signed division already ... >>>>>> >>>>>> SVE doesn't support division by immediates, so SDIV only takes a >>>>>> register. >>>>>> we could always extend the sdiv pattern to support immediates, but it >>>>>> does feel odd given we have a different optab for this. >>>>>> >>>>>> Thanks, >>>>>> Tamar >>>>>> >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>>>> Thanks, >>>>>>>> Tamar >>>>>>>> >>>>>>>>> >>>>>>>>> Richard. >>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Tamar >>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Soumya >>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Tamar >>>>>>>>>>>> >>>>>>>>>>>> + } >>>>>>>>>>>> + } >>>>>>>>>>>> + } >>>>>>>>>>>> if (op1_is_constant) >>>>>>>>>>>> { >>>>>>>>>>>> scalar_int_mode int_mode = as_a <scalar_int_mode> >>>>>>>>>>> (compute_mode); >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Richard Biener <[email protected]> >>>>>>>>> SUSE Software Solutions Germany GmbH, >>>>>>>>> Frankenstrasse 146, 90461 Nuernberg, Germany; >>>>>>>>> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG >>>>>>>>> Nuernberg) >>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Richard Biener <[email protected]> >>>>>>> SUSE Software Solutions Germany GmbH, >>>>>>> Frankenstrasse 146, 90461 Nuernberg, Germany; >>>>>>> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG >>>>>>> Nuernberg)
v2-0001-aarch64-Use-SVE-ASRD-with-vector-division-using-divi.patch
Description: v2-0001-aarch64-Use-SVE-ASRD-with-vector-division-using-divi.patch
