Re: [PATCH] aarch64: Fix invalid CSE for RDRFFS [PR 126629]
Alfie Richards <[email protected]>
| Newsgroups | gmane.comp.gcc.patches |
|---|---|
| Message-ID | <[email protected]> |
On 10/08/2026 10:23, Tamar Christina wrote: > The 08/10/2026 10:19, Alfie Richards wrote: >> Hi Tamar, >> >> (Sorry on my phone again) >> >>> For that all you need is to use the value result of the load >>> as the unique token. So instead of const_int you have >> >>> (define_insn "aarch64_update_ffr_for_load" >>> [(set (reg:VNx16BI FFR_REGNUM) >>> (unspec:VNx16BI [(reg:VNx16BI FFRT_REGNUM) >>> (reg:VNx16BI FFR_REGNUM) >>> (match_operand 0 "register_operand" "w")] >>> UNSPEC_UPDATE_FFR))] >> >> I did consider this too but my concern then is that you force the load values live even if the results aren’t used. (Provided the ffr stays live which it often does) So we may lose DCE on the load. >> >> Sorry to be so picky about this, I’m aware it doesn’t really matter that much. > > The only reason the loads would stay live is because the FFR group as a whole is live, > which means you have another load in same group which the result is needed. > > But then you can't CSE the unused load because even though it's result isn't used it > influences the FFR. > > So I don't think the situation you described above is valid to remove the load as FFRs > can only be CSE'd as a group, or not at all. Yeah fair point, I would consider this a bit of a grey area personally, but I dont think either interpretation is wrong. And neither are too important either as there aren't many valid situations I can imagine where a user cares about the FFR but not the loaded value. This approach seems like it will work well then. It will cause issues for the HSSR patches as it will prevent combine/forwprop creating widening loads from the partial load + widens, but that can probably be gotten around other ways... hopefully. Will respin with this approach, Thanks, Alfie > > Thanks, > Tamar > >> >> I think in balance the max_uid solution may be best? Happy to re-spin with that when I’m back. >> >> Thanks, >> Alfie >> >> >> ________________________________ >> From: Tamar Christina <[email protected]> >> Sent: 10 August 2026 08:10 >> To: Alfie Richards <[email protected]> >> Cc: [email protected] <[email protected]>; Alex Coplan <[email protected]>; Alice Carlotti <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Richard Earnshaw <[email protected]>; Wilco Dijkstra <[email protected]>; [email protected] <[email protected]> >> Subject: RE: [PATCH] aarch64: Fix invalid CSE for RDRFFS [PR 126629] >> >> Hi Alfie, >> >>> -----Original Message----- >>> From: Alfie Richards <[email protected]> >>> Sent: 07 August 2026 14:05 >>> To: Tamar Christina <[email protected]> >>> Cc: [email protected]; Alex Coplan <[email protected]>; Alice >>> Carlotti <[email protected]>; [email protected]; >>> [email protected]; Richard Earnshaw <[email protected]>; >>> Wilco Dijkstra <[email protected]>; [email protected] >>> Subject: Re: [PATCH] aarch64: Fix invalid CSE for RDRFFS [PR 126629] >>> >>> On 07/08/2026 13:16, Tamar Christina wrote: >>>> Hi Alfie, >>>> >>>> The 08/07/2026 11:31, Alfie Richards wrote: >>>>> Hi Tamar, >>>>> Sorry replying from phone so poor formatting. >>>>> >>>>>> I think the idea is good, but I don't quite like that now every first faulting >>> load shape >>>>>> needs Its own new update ffr and that the same UNSPEC has different >>> amount of arguments. >>>>> >>>>> I do agree this is the part of the patch I was most uncomfortable with. >>>>> >>>>>> And because these are pseudo registers and we only have one of them the >>> REGNOs are the same >>>>>> and so it assumes they are the same. The patch fixes it by adding the load >>> arguments to the >>>>>> UNSPEC_UPDATE_FFR to make them unique to the load. >>>>> >>>>> Essentially yes but it’s also because the whole FFR expression tree matches >>> going back to the setffr instructions (I believe) >>>>> >>>>>> That should give us a unique aarch64_update_ffr_for_load and also like >>> your patch still >>>>>> allow removals of redundant setffrs and rdffrs but be a bit simpler and >>> easier to backport. >>>>> >>>>> Yeah I like that idea, and I think it should work. I did think about something >>> similar but my only hesitation was around the situation where you do two >>> identical loads. I like in that situation with my patch that the duplicate load and >>> rdffr gets removed whereas with the solution you propose we would remove >>> the ldffr but keep the duplicate rdffr (and hidden non-instructions). Not a big >>> issue as it’s a pretty nonsense case but that’s why I went the way I did. >>>>> >>>> >>>> I'm not sure I understand which scenario this is. >>>> >>>> Applying your patch: >>>> >>>> #include <arm_sve.h> >>>> >>>> void >>>> same_load_read_same_load_read (unsigned char *p, unsigned char *o1, >>>> unsigned char *o2, unsigned long *r1, >>>> unsigned long *r2) >>>> { >>>> svbool_t pt = svptrue_b8 (); >>>> >>>> svsetffr (); >>>> >>>> svuint8_t a = svldff1_u8 (pt, p); >>>> unsigned long n1 = svcntp_b8 (pt, svrdffr ()); >>>> >>>> svuint8_t b = svldff1_u8 (pt, p); >>>> unsigned long n2 = svcntp_b8 (pt, svrdffr ()); >>>> >>>> svst1_u8 (pt, o1, a); >>>> svst1_u8 (pt, o2, b); >>>> *r1 = n1; >>>> *r2 = n2; >>>> } >>> >>> Ah so it would need to be the same situation as the original case with >>> the double setffr, which is important to make the "expression" trees >>> identical. I guess this make this opimiation so restrictive to the point >>> of being even more negligable. >>> >>> So: >>> >>> #include <arm_sve.h> >>> >>> void >>> same_load_read_same_load_read (unsigned char *p, unsigned char *o1, >>> unsigned char *o2, unsigned long*r1, >>> unsigned long *r2) >>> { >>> svbool_t pt = svptrue_b8 (); >>> >>> svsetffr (); >>> >>> svuint8_t a = svldff1_u8 (pt, p); >>> unsigned long n1 = svcntp_b8 (pt, svrdffr ()); >>> >>> svsetffr (); >>> >>> svuint8_t b = svldff1_u8 (pt, p); >>> unsigned long n2 = svcntp_b8 (pt, svrdffr ()); >>> >>> svst1_u8 (pt, o1, a); >>> svst1_u8 (pt, o2, b); >>> *r1 = n1; >>> *r2 = n2; >>> } >>> >>> Gives: >>> >>> same_load_read_same_load_read: >>> .LFB2: >>> .cfi_startproc >>> ptrue p7.b, all >>> setffr >>> ldff1b z31.b, p7/z, [x0] >>> st1b z31.b, p7, [x1] >>> rdffr p15.b >>> cntp x5, p7, p15.b >>> st1b z31.b, p7, [x2] >>> str x5, [x3] >>> str x5, [x4] >>> ret >>> .cfi_endproc >>> >>> But again, pretty niche case. >> >> For that all you need is to use the value result of the load >> as the unique token. So instead of const_int you have >> >> (define_insn "aarch64_update_ffr_for_load" >> [(set (reg:VNx16BI FFR_REGNUM) >> (unspec:VNx16BI [(reg:VNx16BI FFRT_REGNUM) >> (reg:VNx16BI FFR_REGNUM) >> (match_operand 0 "register_operand" "w")] >> UNSPEC_UPDATE_FFR))] >> >> And then the emitters become >> >> rtx res = e.use_contiguous_load_insn (icode); >> emit_insn (gen_aarch64_update_ffr_for_load (res)); >> return res; >> >> etc. When the optimizers prove the loads are the same the results >> become the same pseudos or get an equivalent relationship set which >> makes the UNSPEC_UPDATE_FFRs the same and so they can be CSEd. >> >> Thanks, >> Tamar >> >>> >>> Thanks, >>> Alfie >>>> >>>> Or did I misunderstand the case you were talking about? >>>> >>>> Thanks, >>>> Tamar >>>> >>>>> Thoughts? Happy to take your solution if you’d still rather. >>>>> >>>>> KR, >>>>> Alfie >>>>> >>>>> ________________________________ >>>>> From: Tamar Christina <[email protected]> >>>>> Sent: 07 August 2026 09:23 >>>>> To: Alfie Richards <[email protected]>; [email protected] >>> <[email protected]> >>>>> Cc: Alfie Richards <[email protected]>; Alex Coplan >>> <[email protected]>; Alice Carlotti <[email protected]>; >>> [email protected] <[email protected]>; >>> [email protected] <[email protected]>; Richard Earnshaw >>> <[email protected]>; Wilco Dijkstra <[email protected]>; >>> [email protected] <[email protected]> >>>>> Subject: RE: [PATCH] aarch64: Fix invalid CSE for RDRFFS [PR 126629] >>>>> >>>>>> -----Original Message----- >>>>>> From: Alfie Richards <[email protected]> >>>>>> Sent: 05 August 2026 16:42 >>>>>> To: [email protected] >>>>>> Cc: Alfie Richards <[email protected]>; Alex Coplan >>>>>> <[email protected]>; Alice Carlotti <[email protected]>; >>>>>> [email protected]; [email protected]; Richard >>> Earnshaw >>>>>> <[email protected]>; Tamar Christina >>> <[email protected]>; >>>>>> Wilco Dijkstra <[email protected]>; [email protected] >>>>>> Subject: [PATCH] aarch64: Fix invalid CSE for RDRFFS [PR 126629] >>>>>> >>>>>> Fixes the invalid CSE'ing of the FFR reads by adding the arguments from >>>>>> the FFR loads to the associated FFR updates. >>>>>> >>>>>> This isn't quite perfectly true, as the hardware presumably could set >>>>>> return different FFR values from the exact same load happening twice, but >>> that >>>>>> is a nonsense situation where the optimisation is valid. >>>>>> >>>>> >>>>> Hi Alfie, >>>>> >>>>> I think the idea is good, but I don't quite like that now every first faulting >>> load shape >>>>> needs Its own new update ffr and that the same UNSPEC has different >>> amount of arguments. >>>>> >>>>> If I’m not mistaken the problem is that CSE lib essentially sees >>>>> >>>>> (set (reg:VNx16BI FFR_REGNUM) >>>>> (unspec:VNx16BI [(reg:VNx16BI FFRT_REGNUM) >>>>> (reg:VNx16BI FFR_REGNUM)] >>>>> UNSPEC_UPDATE_FFR)) >>>>> (set (reg:VNx16BI FFR_REGNUM) >>>>> (unspec:VNx16BI [(reg:VNx16BI FFRT_REGNUM) >>>>> (reg:VNx16BI FFR_REGNUM)] >>>>> UNSPEC_UPDATE_FFR)) >>>>> >>>>> And because these are pseudo registers and we only have one of them the >>> REGNOs are the same >>>>> and so it assumes they are the same. The patch fixes it by adding the load >>> arguments to the >>>>> UNSPEC_UPDATE_FFR to make them unique to the load. >>>>> >>>>> But I think we can fix this simpler by just adding a unique token to the >>> aarch64_update_ffr_for_load. >>>>> >>>>> Like >>>>> >>>>> (define_insn "aarch64_update_ffr_for_load" >>>>> [(set (reg:VNx16BI FFR_REGNUM) >>>>> (unspec:VNx16BI [(reg:VNx16BI FFRT_REGNUM) >>>>> (reg:VNx16BI FFR_REGNUM) >>>>> (match_operand 0 "const_int_operand" "n")] >>>>> UNSPEC_UPDATE_FFR))] >>>>> >>>>> Then change >>>>> >>>>> emit_insn (gen_aarch64_update_ffr_for_load ()); >>>>> >>>>> into >>>>> >>>>> emit_insn (gen_aarch64_update_ffr_for_load (GEN_INT (get_max_uid ()))); >>>>> >>>>> the exact value of which doesn't really matter as long as it's unique (maybe >>> make a helper). >>>>> >>>>> That should give us a unique aarch64_update_ffr_for_load and also like >>> your patch still >>>>> allow removals of redundant setffrs and rdffrs but be a bit simpler and >>> easier to backport. >>>>> >>>>> Thanks, >>>>> Tamar >>>>> >>>>>> PR 126629 >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> * config/aarch64/aarch64-sve-builtins-base.cc: >>>>>> (svldff1_gather_impl::expand): Remove >>>>>> gen_aarch64_update_ffr_for_load. >>>>>> (svldff1_svldff1_gather_extend::expand): Ditto. >>>>>> (svldxf1_impl::expand): Ditto. >>>>>> (svldxf1_extend_impl::expand): Ditto. >>>>>> * config/aarch64/aarch64-sve.md (aarch64_update_ffr_for_load): >>>>>> Remove. >>>>>> (*aarch64_update_ffr_for_continuous_load): New. >>>>>> (*aarch64_update_ffr_for_gather_load): New. >>>>>> (@aarch64_ld<fn>f1<mode>): New. >>>>>> (*aarch64_ld<fn>f1<mode>): Change to not be expanded directly. >>>>>> (@aarch64_ld<fn>f1_<ANY_EXTEND:optab><SVE_HSDI:mode><SVE_ >>>>>> PARTIAL_I:mode>): New. >>>>>> (*aarch64_ld<fn>f1_<ANY_EXTEND:optab><SVE_HSDI:mode><SVE_P >>>>>> ARTIAL_I:mode>): Change to not be expanded directly. >>>>>> (@aarch64_ldff1_gather<mode>): New. >>>>>> (*aarch64_ldff1_gather<mode>): Change to not be expanded directly. >>>>>> (@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx4_WIDE:mode> >>>>>> <VNx4_NARROW:mode>): New. >>>>>> (*aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx4_WIDE:mode>< >>>>>> VNx4_NARROW:mode>): Change to not be expanded directly. >>>>>> (@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx2_WIDE:mode> >>>>>> <VNx2_NARROW:mode>): New. >>>>>> (*aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx2_WIDE:mode>< >>>>>> VNx2_NARROW:mode>): Change to not be expanded directly. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> * gcc.target/aarch64/sve/pr126629.c: New test. >>>>>> * gcc.target/aarch64/sve/pr126629_extend.c: New test. >>>>>> * gcc.target/aarch64/sve/pr126629_gather.c: New test. >>>>>> * gcc.target/aarch64/sve/pr126629_gather2.c: New test. >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> Bootstrapped and regression tested on aarch64. >>>>>> >>>>>> Okay for master and backport? >>>>>> >>>>>> Thanks, >>>>>> Alfie >>>>>> >>>>>> --- >>>>>> .../aarch64/aarch64-sve-builtins-base.cc | 4 - >>>>>> gcc/config/aarch64/aarch64-sve.md | 206 ++++++++++++++++-- >>>>>> .../gcc.target/aarch64/sve/pr126629.c | 23 ++ >>>>>> .../gcc.target/aarch64/sve/pr126629_extend.c | 23 ++ >>>>>> .../gcc.target/aarch64/sve/pr126629_gather.c | 24 ++ >>>>>> .../gcc.target/aarch64/sve/pr126629_gather2.c | 24 ++ >>>>>> 6 files changed, 279 insertions(+), 25 deletions(-) >>>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr126629.c >>>>>> create mode 100644 >>>>>> gcc/testsuite/gcc.target/aarch64/sve/pr126629_extend.c >>>>>> create mode 100644 >>>>>> gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather.c >>>>>> create mode 100644 >>>>>> gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather2.c >>>>>> >>>>>> diff --git a/gcc/config/aarch64/aarch64-sve-builtins-base.cc >>>>>> b/gcc/config/aarch64/aarch64-sve-builtins-base.cc >>>>>> index 7f047bb6468..186f223ac63 100644 >>>>>> --- a/gcc/config/aarch64/aarch64-sve-builtins-base.cc >>>>>> +++ b/gcc/config/aarch64/aarch64-sve-builtins-base.cc >>>>>> @@ -2001,7 +2001,6 @@ public: >>>>>> { >>>>>> /* See the block comment in aarch64-sve.md for details about the >>>>>> FFR handling. */ >>>>>> - emit_insn (gen_aarch64_update_ffr_for_load ()); >>>>>> >>>>>> e.prepare_gather_address_operands (1); >>>>>> /* Put the predicate last, since ldff1_gather uses the same operand >>>>>> @@ -2023,7 +2022,6 @@ public: >>>>>> { >>>>>> /* See the block comment in aarch64-sve.md for details about the >>>>>> FFR handling. */ >>>>>> - emit_insn (gen_aarch64_update_ffr_for_load ()); >>>>>> >>>>>> e.prepare_gather_address_operands (1); >>>>>> /* Put the predicate last, since ldff1_gather uses the same operand >>>>>> @@ -2075,7 +2073,6 @@ public: >>>>>> { >>>>>> /* See the block comment in aarch64-sve.md for details about the >>>>>> FFR handling. */ >>>>>> - emit_insn (gen_aarch64_update_ffr_for_load ()); >>>>>> >>>>>> machine_mode mode = e.vector_mode (0); >>>>>> return e.use_contiguous_load_insn (code_for_aarch64_ldf1 >>> (m_unspec, >>>>>> mode)); >>>>>> @@ -2103,7 +2100,6 @@ public: >>>>>> { >>>>>> /* See the block comment in aarch64-sve.md for details about the >>>>>> FFR handling. */ >>>>>> - emit_insn (gen_aarch64_update_ffr_for_load ()); >>>>>> >>>>>> insn_code icode = code_for_aarch64_ldf1 (m_unspec, extend_rtx_code >>> (), >>>>>> e.vector_mode (0), >>>>>> diff --git a/gcc/config/aarch64/aarch64-sve.md >>>>>> b/gcc/config/aarch64/aarch64-sve.md >>>>>> index 105b34eb8fa..1e131cc404a 100644 >>>>>> --- a/gcc/config/aarch64/aarch64-sve.md >>>>>> +++ b/gcc/config/aarch64/aarch64-sve.md >>>>>> @@ -1150,10 +1150,31 @@ (define_insn "aarch64_wrffr" >>>>>> ;; so that the FFR value is live on entry to the region and so that the FFR >>>>>> ;; value visibly changes within the region. This is used (possibly multiple >>>>>> ;; times) in an FFRT region that includes LDFF1 or LDNF1 instructions. >>>>>> -(define_insn "aarch64_update_ffr_for_load" >>>>>> +(define_insn "*aarch64_update_ffr_for_continuous_load" >>>>>> [(set (reg:VNx16BI FFR_REGNUM) >>>>>> - (unspec:VNx16BI [(reg:VNx16BI FFRT_REGNUM) >>>>>> - (reg:VNx16BI FFR_REGNUM)] >>>>>> UNSPEC_UPDATE_FFR))] >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_operand 0 "" "X") >>>>>> + (match_operand 1 "" "X") >>>>>> + ] UNSPEC_UPDATE_FFR))] >>>>>> + "TARGET_SVE" >>>>>> + "" >>>>>> + [(set_attr "type" "no_insn")] >>>>>> +) >>>>>> + >>>>>> +(define_insn "*aarch64_update_ffr_for_gather_load" >>>>>> + [(set (reg:VNx16BI FFR_REGNUM) >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_operand 0 "" "X") >>>>>> + (match_operand 1 "" "X") >>>>>> + (match_operand 2 "" "X") >>>>>> + (match_operand 3 "" "X") >>>>>> + (match_operand 4 "" "X") >>>>>> + ] UNSPEC_UPDATE_FFR) >>>>>> + )] >>>>>> "TARGET_SVE" >>>>>> "" >>>>>> [(set_attr "type" "no_insn")] >>>>>> @@ -1448,7 +1469,26 @@ (define_insn_and_rewrite >>>>>> "*aarch64_load_<ANY_EXTEND:optab>_mov<SVE_HSDI:mode><SV >>>>>> ;; ------------------------------------------------------------------------- >>>>>> >>>>>> ;; Contiguous non-extending first-faulting or non-faulting loads. >>>>>> -(define_insn "@aarch64_ld<fn>f1<mode>" >>>>>> +(define_expand "@aarch64_ld<fn>f1<mode>" >>>>>> + [(set (reg:VNx16BI FFR_REGNUM) ; Update the FFR >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_dup 2) >>>>>> + (match_dup 1) >>>>>> + ] UNSPEC_UPDATE_FFR) >>>>>> + ) >>>>>> + (set (match_operand:SVE_FULL 0 "register_operand") >>>>>> + (unspec:SVE_FULL >>>>>> + [(match_operand:<VPRED> 2 "register_operand") >>>>>> + (match_operand:SVE_FULL 1 "aarch64_sve_ld<fn>f1_operand") >>>>>> + (reg:VNx16BI FFRT_REGNUM)] >>>>>> + SVE_LDFF1_LDNF1))] >>>>>> + "TARGET_SVE && TARGET_NON_STREAMING" >>>>>> + {} >>>>>> +) >>>>>> + >>>>>> +(define_insn "*aarch64_ld<fn>f1<mode>" >>>>>> [(set (match_operand:SVE_FULL 0 "register_operand" "=w") >>>>>> (unspec:SVE_FULL >>>>>> [(match_operand:<VPRED> 2 "register_operand" "Upl") >>>>>> @@ -1479,7 +1519,36 @@ (define_insn "@aarch64_ld<fn>f1<mode>" >>>>>> ;; ------------------------------------------------------------------------- >>>>>> >>>>>> ;; Predicated first-faulting or non-faulting load and extend. >>>>>> -(define_insn_and_rewrite >>>>>> >>> "@aarch64_ld<fn>f1_<ANY_EXTEND:optab><SVE_HSDI:mode><SVE_PARTIAL >>>>>> _I:mode>" >>>>>> +(define_expand >>>>>> >>> "@aarch64_ld<fn>f1_<ANY_EXTEND:optab><SVE_HSDI:mode><SVE_PARTIAL >>>>>> _I:mode>" >>>>>> + [(set (reg:VNx16BI FFR_REGNUM) ; Update the FFR >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_dup 2) >>>>>> + (match_dup 1) >>>>>> + ] UNSPEC_UPDATE_FFR) >>>>>> + ) >>>>>> + (set (match_operand:SVE_HSDI 0 "register_operand") >>>>>> + (unspec:SVE_HSDI >>>>>> + [(match_operand:<SVE_HSDI:VPRED> 3 "general_operand") >>>>>> + (ANY_EXTEND:SVE_HSDI >>>>>> + (unspec:SVE_PARTIAL_I >>>>>> + [(match_operand:<SVE_PARTIAL_I:VPRED> 2 "register_operand") >>>>>> + (match_operand:SVE_PARTIAL_I 1 >>>>>> "aarch64_sve_ld<fn>f1_operand") >>>>>> + (reg:VNx16BI FFRT_REGNUM)] >>>>>> + SVE_LDFF1_LDNF1))] >>>>>> + UNSPEC_PRED_X))] >>>>>> + "TARGET_SVE >>>>>> + && TARGET_NON_STREAMING >>>>>> + && (~<SVE_HSDI:narrower_mask> & <SVE_PARTIAL_I:self_mask>) == >>> 0" >>>>>> + { >>>>>> + if (!CONSTANT_P (operands[3])) >>>>>> + operands[3] = CONSTM1_RTX (<SVE_HSDI:VPRED>mode); >>>>>> + } >>>>>> +) >>>>>> + >>>>>> +;; Predicated first-faulting or non-faulting load and extend. >>>>>> +(define_insn >>>>>> >>> "*aarch64_ld<fn>f1_<ANY_EXTEND:optab><SVE_HSDI:mode><SVE_PARTIAL_ >>>>>> I:mode>" >>>>>> [(set (match_operand:SVE_HSDI 0 "register_operand" "=w") >>>>>> (unspec:SVE_HSDI >>>>>> [(match_operand:<SVE_HSDI:VPRED> 3 "general_operand" >>>>>> "UplDnm") >>>>>> @@ -1494,10 +1563,6 @@ (define_insn_and_rewrite >>>>>> "@aarch64_ld<fn>f1_<ANY_EXTEND:optab><SVE_HSDI:mode><SV >>>>>> && TARGET_NON_STREAMING >>>>>> && (~<SVE_HSDI:narrower_mask> & <SVE_PARTIAL_I:self_mask>) == 0" >>>>>> >>>>>> >>> "ld<fn>f1<ANY_EXTEND:s><SVE_PARTIAL_I:Vesize>\t%0.<SVE_HSDI:Vctype>, >>>>>> %2/z, %1" >>>>>> - "&& !CONSTANT_P (operands[3])" >>>>>> - { >>>>>> - operands[3] = CONSTM1_RTX (<SVE_HSDI:VPRED>mode); >>>>>> - } >>>>>> [(set_attr "sve_type" "sve_load_1reg")] >>>>>> ) >>>>>> >>>>>> @@ -1907,7 +1972,33 @@ (define_insn_and_rewrite >>>>>> "*aarch64_gather_load_<ANY_EXTEND:optab><SVE_2HSDI:mode >>>>>> >>>>>> ;; Predicated first-faulting gather loads for 32-bit elements. Operand >>>>>> ;; 3 is true for unsigned extension and false for signed extension. >>>>>> -(define_insn "@aarch64_ldff1_gather<mode>" >>>>>> +(define_expand "@aarch64_ldff1_gather<mode>" >>>>>> + [(set (reg:VNx16BI FFR_REGNUM) ; Update the FFR >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_dup 5) >>>>>> + (match_dup 1) >>>>>> + (match_dup 2) >>>>>> + (match_dup 3) >>>>>> + (match_dup 4) >>>>>> + ] UNSPEC_UPDATE_FFR) >>>>>> + ) >>>>>> + (set (match_operand:SVE_FULL_S 0 "register_operand") ; The actual >>> load >>>>>> + (unspec:SVE_FULL_S >>>>>> + [(match_operand:VNx4BI 5 "register_operand") >>>>>> + (match_operand:DI 1 "aarch64_sve_gather_offset_w") >>>>>> + (match_operand:VNx4SI 2 "register_operand") >>>>>> + (match_operand:DI 3 "const_int_operand") >>>>>> + (match_operand:DI 4 "aarch64_gather_scale_operand_w") >>>>>> + (mem:BLK (scratch)) >>>>>> + (reg:VNx16BI FFRT_REGNUM)] >>>>>> + UNSPEC_LDFF1_GATHER))] >>>>>> + "TARGET_SVE && TARGET_NON_STREAMING" >>>>>> + {} >>>>>> +) >>>>>> + >>>>>> +(define_insn "*aarch64_ldff1_gather<mode>" >>>>>> [(set (match_operand:SVE_FULL_S 0 "register_operand") >>>>>> (unspec:SVE_FULL_S >>>>>> [(match_operand:VNx4BI 5 "register_operand") >>>>>> @@ -1938,7 +2029,33 @@ (define_insn >>> "@aarch64_ldff1_gather<mode>" >>>>>> >>>>>> ;; Predicated first-faulting gather loads for 64-bit elements. The value >>>>>> ;; of operand 3 doesn't matter in this case. >>>>>> -(define_insn "@aarch64_ldff1_gather<mode>" >>>>>> +(define_expand "@aarch64_ldff1_gather<mode>" >>>>>> + [(set (reg:VNx16BI FFR_REGNUM) ; Update the FFR >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_dup 5) >>>>>> + (match_dup 1) >>>>>> + (match_dup 2) >>>>>> + (match_dup 3) >>>>>> + (match_dup 4) >>>>>> + ] UNSPEC_UPDATE_FFR) >>>>>> + ) >>>>>> + (set (match_operand:SVE_FULL_D 0 "register_operand") >>>>>> + (unspec:SVE_FULL_D >>>>>> + [(match_operand:VNx2BI 5 "register_operand") >>>>>> + (match_operand:DI 1 "aarch64_sve_gather_offset_d") >>>>>> + (match_operand:VNx2DI 2 "register_operand") >>>>>> + (match_operand:DI 3 "const_int_operand") >>>>>> + (match_operand:DI 4 "aarch64_gather_scale_operand_d") >>>>>> + (mem:BLK (scratch)) >>>>>> + (reg:VNx16BI FFRT_REGNUM)] >>>>>> + UNSPEC_LDFF1_GATHER))] >>>>>> + "TARGET_SVE && TARGET_NON_STREAMING" >>>>>> + {} >>>>>> +) >>>>>> + >>>>>> +(define_insn "*aarch64_ldff1_gather<mode>" >>>>>> [(set (match_operand:SVE_FULL_D 0 "register_operand") >>>>>> (unspec:SVE_FULL_D >>>>>> [(match_operand:VNx2BI 5 "register_operand") >>>>>> @@ -2032,7 +2149,40 @@ (define_insn >>>>>> "*aarch64_ldff1_gather<mode>_uxtw" >>>>>> >>>>>> ;; Predicated extending first-faulting gather loads for 32-bit elements. >>>>>> ;; Operand 3 is true for unsigned extension and false for signed extension. >>>>>> -(define_insn_and_rewrite >>>>>> >>> "@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx4_WIDE:mode><VNx4_ >>>>>> NARROW:mode>" >>>>>> +(define_expand >>>>>> >>> "@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx4_WIDE:mode><VNx4_ >>>>>> NARROW:mode>" >>>>>> + [(set (reg:VNx16BI FFR_REGNUM) ; Update the FFR >>>>>> + (unspec:VNx16BI >>>>>> + [(reg:VNx16BI FFRT_REGNUM) >>>>>> + (reg:VNx16BI FFR_REGNUM) >>>>>> + (match_dup 5) >>>>>> + (match_dup 1) >>>>>> + (match_dup 2) >>>>>> + (match_dup 3) >>>>>> + (match_dup 4) >>>>>> + ] UNSPEC_UPDATE_FFR) >>>>>> + ) >>>>>> + (set (match_operand:VNx4_WIDE 0 "register_operand") >>>>>> + (unspec:VNx4_WIDE >>>>>> + [(match_operand:VNx4BI 6 "general_operand") >>>>>> + (ANY_EXTEND:VNx4_WIDE >>>>>> + (unspec:VNx4_NARROW >>>>>> + [(match_operand:VNx4BI 5 "register_operand") >>>>>> + (match_operand:DI 1 >>>>>> "aarch64_sve_gather_offset_<VNx4_NARROW:Vesize>") >>>>>> + (match_operand:VNx4_WIDE 2 "register_operand") >>>>>> + (match_operand:DI 3 "const_int_operand") >>>>>> + (match_operand:DI 4 >>>>>> "aarch64_gather_scale_operand_<VNx4_NARROW:Vesize>") >>>>>> + (mem:BLK (scratch)) >>>>>> + (reg:VNx16BI FFRT_REGNUM)] >>>>>> + UNSPEC_LDFF1_GATHER))] >>>>>> + UNSPEC_PRED_X))] >>>>>> + "TARGET_SVE && TARGET_NON_STREAMING" >>>>>> + { >>>>>> + if (!CONSTANT_P (operands[6])) >>>>>> + operands[6] = CONSTM1_RTX (VNx4BImode); >>>>>> + } >>>>>> +) >>>>>> + >>>>>> +(define_insn >>>>>> >>> "*aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx4_WIDE:mode><VNx4_N >>>>>> ARROW:mode>" >>>>>> [(set (match_operand:VNx4_WIDE 0 "register_operand") >>>>>> (unspec:VNx4_WIDE >>>>>> [(match_operand:VNx4BI 6 "general_operand") >>>>>> @@ -2062,16 +2212,34 @@ (define_insn_and_rewrite >>>>>> "@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx4_WIDE:mod >>>>>> [&w, rk, w, Ui1, i, Upl, UplDnm] >>>>>> ldff1<ANY_EXTEND:s><VNx4_NARROW:Vesize>\t%0.s, %5/z, [%1, %2.s, >>> uxtw >>>>>> %p4] >>>>>> [?w, rk, 0, Ui1, i, Upl, UplDnm] ^ >>>>>> } >>>>>> - "&& !CONSTANT_P (operands[6])" >>>>>> - { >>>>>> - operands[6] = CONSTM1_RTX (VNx4BImode); >>>>>> - } >>>>>> [(set_attr "sve_type" "sve_gatherload_32")] >>>>>> ) >>>>>> >>>>>> ;; Predicated extending first-faulting gather loads for 64-bit elements. >>>>>> ;; The value of operand 3 doesn't matter in this case. >>>>>> -(define_insn_and_rewrite >>>>>> >>> "@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx2_WIDE:mode><VNx2_ >>>>>> NARROW:mode>" >>>>>> +(define_expand >>>>>> >>> "@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx2_WIDE:mode><VNx2_ >>>>>> NARROW:mode>" >>>>>> + [(set (match_operand:VNx2_WIDE 0 "register_operand") >>>>>> + (unspec:VNx2_WIDE >>>>>> + [(match_operand:VNx2BI 6 "general_operand") >>>>>> + (ANY_EXTEND:VNx2_WIDE >>>>>> + (unspec:VNx2_NARROW >>>>>> + [(match_operand:VNx2BI 5 "register_operand") >>>>>> + (match_operand:DI 1 >>>>>> "aarch64_sve_gather_offset_<VNx2_NARROW:Vesize>") >>>>>> + (match_operand:VNx2_WIDE 2 "register_operand") >>>>>> + (match_operand:DI 3 "const_int_operand") >>>>>> + (match_operand:DI 4 >>>>>> "aarch64_gather_scale_operand_<VNx2_NARROW:Vesize>") >>>>>> + (mem:BLK (scratch)) >>>>>> + (reg:VNx16BI FFRT_REGNUM)] >>>>>> + UNSPEC_LDFF1_GATHER))] >>>>>> + UNSPEC_PRED_X))] >>>>>> + "TARGET_SVE && TARGET_NON_STREAMING" >>>>>> + { >>>>>> + if (!CONSTANT_P (operands[6])) >>>>>> + operands[6] = CONSTM1_RTX (VNx2BImode); >>>>>> + } >>>>>> +) >>>>>> + >>>>>> +(define_insn >>>>>> >>> "*aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx2_WIDE:mode><VNx2_N >>>>>> ARROW:mode>" >>>>>> [(set (match_operand:VNx2_WIDE 0 "register_operand") >>>>>> (unspec:VNx2_WIDE >>>>>> [(match_operand:VNx2BI 6 "general_operand") >>>>>> @@ -2097,10 +2265,6 @@ (define_insn_and_rewrite >>>>>> "@aarch64_ldff1_gather_<ANY_EXTEND:optab><VNx2_WIDE:mod >>>>>> [&w, rk, w, i, i, Upl, UplDnm] >>>>>> ldff1<ANY_EXTEND:s><VNx2_NARROW:Vesize>\t%0.d, %5/z, [%1, %2.d, >>> lsl >>>>>> %p4] >>>>>> [?w, rk, w, i, i, Upl, UplDnm] ^ >>>>>> } >>>>>> - "&& !CONSTANT_P (operands[6])" >>>>>> - { >>>>>> - operands[6] = CONSTM1_RTX (VNx2BImode); >>>>>> - } >>>>>> [(set_attr "sve_type" "sve_gatherload_64")] >>>>>> ) >>>>>> >>>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr126629.c >>>>>> b/gcc/testsuite/gcc.target/aarch64/sve/pr126629.c >>>>>> new file mode 100644 >>>>>> index 00000000000<tel:00000000000<tel:00000000000>>..64cb3ece330 >>>>>> --- /dev/null >>>>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr126629.c >>>>>> @@ -0,0 +1,23 @@ >>>>>> +/* { dg-do compile } */ >>>>>> +/* { dg-options "-O3" } */ >>>>>> + >>>>>> +#include <arm_sve.h> >>>>>> + >>>>>> + void >>>>>> +two_scans (unsigned char *p, unsigned char *q, unsigned long *r1, >>>>>> + unsigned long *r2, unsigned char *o1, unsigned char *o2) >>>>>> +{ >>>>>> + svbool_t pt = svptrue_b8 (); >>>>>> + svsetffr (); >>>>>> + svuint8_t a = svldff1_u8 (pt, p); >>>>>> + unsigned long n1 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svsetffr (); >>>>>> + svuint8_t b = svldff1_u8 (pt, q); >>>>>> + unsigned long n2 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svst1_u8 (pt, o1, a); >>>>>> + svst1_u8 (pt, o2, b); >>>>>> + *r1 = n1; >>>>>> + *r2 = n2; >>>>>> +} >>>>>> + >>>>>> +/* { dg-final { scan-assembler-times {\trdffr} 2 } } */ >>>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr126629_extend.c >>>>>> b/gcc/testsuite/gcc.target/aarch64/sve/pr126629_extend.c >>>>>> new file mode 100644 >>>>>> index 00000000000<tel:00000000000<tel:00000000000>>..909a9f1e651 >>>>>> --- /dev/null >>>>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr126629_extend.c >>>>>> @@ -0,0 +1,23 @@ >>>>>> +/* { dg-do compile } */ >>>>>> +/* { dg-options "-O3" } */ >>>>>> + >>>>>> +#include <arm_sve.h> >>>>>> + >>>>>> + void >>>>>> +two_scans (unsigned char *p, unsigned char *q, unsigned long *r1, >>>>>> + unsigned long *r2, unsigned int *o1, unsigned int *o2) >>>>>> +{ >>>>>> + svbool_t pt = svptrue_b8 (); >>>>>> + svsetffr (); >>>>>> + svuint32_t a = svldff1ub_u32 (pt, p); >>>>>> + unsigned long n1 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svsetffr (); >>>>>> + svuint32_t b = svldff1ub_u32 (pt, q); >>>>>> + unsigned long n2 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svst1_u32 (pt, o1, a); >>>>>> + svst1_u32 (pt, o2, b); >>>>>> + *r1 = n1; >>>>>> + *r2 = n2; >>>>>> +} >>>>>> + >>>>>> +/* { dg-final { scan-assembler-times {\trdffr} 2 } } */ >>>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather.c >>>>>> b/gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather.c >>>>>> new file mode 100644 >>>>>> index 00000000000<tel:00000000000<tel:00000000000>>..2348082fde5 >>>>>> --- /dev/null >>>>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather.c >>>>>> @@ -0,0 +1,24 @@ >>>>>> +/* { dg-do compile } */ >>>>>> +/* { dg-options "-O3" } */ >>>>>> + >>>>>> +#include <arm_sve.h> >>>>>> + >>>>>> + void >>>>>> +two_scans (unsigned char *p, unsigned char *q, unsigned long *r1, >>>>>> + unsigned long *r2, unsigned *o1, unsigned *o2, >>>>>> + svuint32_t offset1, svuint32_t offset2) >>>>>> +{ >>>>>> + svbool_t pt = svptrue_b8 (); >>>>>> + svsetffr (); >>>>>> + svuint32_t a = svldff1sb_gather_offset_u32 (pt, p, offset1); >>>>>> + unsigned long n1 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svsetffr (); >>>>>> + svuint32_t b = svldff1sb_gather_offset_u32 (pt, q, offset2); >>>>>> + unsigned long n2 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svst1_u32 (pt, o1, a); >>>>>> + svst1_u32 (pt, o2, b); >>>>>> + *r1 = n1; >>>>>> + *r2 = n2; >>>>>> +} >>>>>> + >>>>>> +/* { dg-final { scan-assembler-times {\trdffr} 2 } } */ >>>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather2.c >>>>>> b/gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather2.c >>>>>> new file mode 100644 >>>>>> index 00000000000<tel:00000000000<tel:00000000000>>..bb9257fe382 >>>>>> --- /dev/null >>>>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr126629_gather2.c >>>>>> @@ -0,0 +1,24 @@ >>>>>> +/* { dg-do compile } */ >>>>>> +/* { dg-options "-O3" } */ >>>>>> + >>>>>> +#include <arm_sve.h> >>>>>> + >>>>>> + void >>>>>> +two_scans (svuint32_t p, svuint32_t q, unsigned long *r1, >>>>>> + unsigned long *r2, unsigned *o1, unsigned *o2, >>>>>> + long int offset1, long int offset2) >>>>>> +{ >>>>>> + svbool_t pt = svptrue_b8 (); >>>>>> + svsetffr (); >>>>>> + svuint32_t a = svldff1sb_gather_u32base_offset_u32 (pt, p, offset1); >>>>>> + unsigned long n1 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svsetffr (); >>>>>> + svuint32_t b = svldff1sb_gather_u32base_offset_u32 (pt, q, offset2); >>>>>> + unsigned long n2 = svcntp_b8 (pt, svrdffr ()); >>>>>> + svst1_u32 (pt, o1, a); >>>>>> + svst1_u32 (pt, o2, b); >>>>>> + *r1 = n1; >>>>>> + *r2 = n2; >>>>>> +} >>>>>> + >>>>>> +/* { dg-final { scan-assembler-times {\trdffr} 2 } } */ >>>>>> -- >>>>>> 2.34.1 >>>>> >>>> >> >