> On 21 May 2026, at 10:27, Tamar Christina <[email protected]> wrote:
>
> Hi Kyrill,
>
>> -----Original Message-----
>> From: Kyrylo Tkachov <[email protected]>
>> Sent: 21 May 2026 09:18
>> To: Tamar Christina <[email protected]>
>> Cc: [email protected]; nd <[email protected]>; Richard Earnshaw
>> <[email protected]>; [email protected]; Alex Coplan
>> <[email protected]>; [email protected]; Wilco Dijkstra
>> <[email protected]>; Alice Carlotti <[email protected]>
>> Subject: Re: [PATCH]AArch64: fix the SVE->SIMD lowering optimization
>> [PR125148]
>>
>> Hi Tamar,
>>
>>> On 20 May 2026, at 20:30, Tamar Christina <[email protected]>
>> wrote:
>>>
>>> The optimization added in
>> g:210d06502f22964c7214586c54f8eb54a6965bfd has an
>>> implementation bug which makes it generate bogus code.
>>>
>>> The optimization was support to convert SVE loads with a known predicate
>> into
>>> Adv. SIMD loads without the predicate.
>>>
>>> The current implementation is done at expansion time where the predicate is
>>> still clearly available.
>>>
>>> It does this by rewriting the loads to an Adv. SIMD load and then taking a
>>> paradoxical subreg of the result into an SVE vector.
>>>
>>> i.e. (subreg:VNx16QI (reg:QI 111) 0) for a byte load with a VL1 predicate.
>>>
>>> The issue is that the SVE loads were UNSPEC before and they didn't get
>> optimized
>>> by passes like forwprop and cse. Adv. SIMD loads are.
>>>
>>> as such in cases where you have such a pattern:
>>>
>>> char[] p = {1,2,3,3};
>>> load (p, VL1)
>>>
>>> we used to generate
>>>
>>> mov w0, 1
>>> strb w0, [x19]
>>> ptrue p7.b, vl1
>>> ld1b z30.b, p7/z, [x19]
>>>
>>> which was dumb, but valid and the above optimization now gets the load
>>> eliminated and the constants folded. However, in particular for scalars,
>>> AArch64 has an optimization that's been a long for ages in which scalar FPR
>>> constants are created using vector broadcasting operations. It assumes
>> scalars
>>> are accessed as scalars (as in, in the mode that created them).
>>>
>>> So the above gets optimized to
>>>
>>> movi v30.8b, 0x1
>>>
>>> which is invalid. The original load requires the inactive elements to be
>>> zero,
>>> where-as by using the paradoxical subreg it's relying on the implicit (as
>>> in,
>>> not modelled in RTL) assumption that the load zeros the top bits, but
>>> doesn't
>>> keep in mind that the load can be optimized away.
>>>
>>> This patch fixes it by creating a full SVE vector of 0s and writing only the
>>> values we want to set using an INSR. (i.e. using VL2 of bytes writes a
>>> short).
>>>
>>> It then provides patterns to optimize this:
>>>
>>> 1. if it's still following a load, just emit the load.
>>> 2. if it's not, then optimize it to a zero'ing operation. so e.g. HI mode
>>> issues an fmov h0, h0 and so clears the top bits to zero.
>>>
>>> I choose this representation because even without the above operations it is
>>> semantically valid and will generate correct code.
>>>
>>> The alternative would be to delay this optimization to e.g. combine however
>> we
>>> have two problems there:
>>>
>>> 1. It's quite late, so the above constant cases for instance don't get
>>> optimized
>>> and we keep the pointless store and loads.
>>> 2. Our RTX costs don't model predicates. and so it may not accept the
>>> combination since the replacement is more expensive.
>>>
>>> So I chose to keep the optimization early, but just replace the paradoxical
>>> subreg with a zero-extend.
>>>
>>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>>>
>>> Ok for master?
>>>
>>> Thanks,
>>> Tamar
>>>
>>> gcc/ChangeLog:
>>>
>>> PR target/125148
>>> * config/aarch64/aarch64-sve.md
>>> (*aarch64_vec_shl_insert_into_zero_<mode>,
>>> *aarch64_vec_shl_insert_into_zero_vnx16qi,
>>> *aarch64_vec_shl_insert_from_load_<mode>): New.
>>> * config/aarch64/aarch64.cc (aarch64_emit_load_store_through_mode):
>>> Replace paradoxical subreg with zero-extend.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> PR target/125148
>>> * gcc.target/aarch64/sve/highway_run.c: New test.
>>>
>>> ---
>>> diff --git a/gcc/config/aarch64/aarch64-sve.md
>> b/gcc/config/aarch64/aarch64-sve.md
>>> index
>> 019630eb8d21941b0ca718838ae6ef23e8aaedab..fc98f47d746019959baa6
>> a8aa29d6b996d8b4822 100644
>>> --- a/gcc/config/aarch64/aarch64-sve.md
>>> +++ b/gcc/config/aarch64/aarch64-sve.md
>>> @@ -3004,6 +3004,51 @@ (define_insn "vec_shl_insert_<mode>"
>>> [(set_attr "sve_type" "sve_int_general")]
>>> )
>>>
>>> +;; Shift an SVE vector left and insert a scalar into element 0 of a zero
>>> +;; register.
>>> +(define_insn "*aarch64_vec_shl_insert_into_zero_<mode>"
>>> + [(set (match_operand:SVE_FULL_HSD 0 "register_operand")
>>> + (unspec:SVE_FULL_HSD
>>> + [(match_operand:SVE_FULL_HSD 1 "aarch64_simd_imm_zero")
>>> + (match_operand:<VEL> 2 "register_operand")]
>>> + UNSPEC_INSR))]
>>> + "TARGET_SVE"
>>> + {@ [ cons: =0 , 1 , 2 ; attrs: movprfx ]
>>> + [ w , Dz , w ; * ] fmov\t%<Vetype>0, %<Vetype>2
>>> + }
>>> + [(set_attr "type" "neon_move")]
>>> +)
>>
>> There’s no need for the movprfx attribute on insns that don’t have a movprfx
>> alternative.
>>
>>> +
>>> +;; Shift an SVE vector left and insert a scalar into element 0 of a zero
>>> +;; register for bytes.
>>> +(define_insn "*aarch64_vec_shl_insert_into_zero_vnx16qi"
>>> + [(set (match_operand:VNx16QI 0 "register_operand")
>>> + (unspec:VNx16QI
>>> + [(match_operand:VNx16QI 1 "aarch64_simd_imm_zero")
>>> + (match_operand:QI 2 "register_operand")]
>>> + UNSPEC_INSR))]
>>> + "TARGET_SVE"
>>> + {@ [ cons: =0 , 1 , 2 ; attrs: movprfx ]
>>> + [ w , Dz , w ; * ] fmov\t%h0, %h2\;and\t%0.h,
>>> %0.h, #0xff
>>> + }
>>> + [(set_attr "type" "neon_move")]
>>> +)
>>
>> This should have the length attribute set to 8 to make sure branch range
>> computations remain valid.
>
> I thought so too, but then I saw the other patterns using \; don't set the
> length
> so I assumed that the machinery does itself. But I'll add the length.
There are other attributes like movprfx that have the effect of setting length
to 8 but if there’s no such attribute and the assembly template emits multiple
assembly instructions, then it’s a bug to not set the length.
>
>> That said, wouldn’t this be better as an insn_and_split so that the “and”
>> operation can be optimized and scheduled separately?
>
> I did it this way to avoid re-introducing the paradoxical subregs again,
> since the above is really playing tricks with the register file.
>
> The alternative was to use an SVE mov instruction, but then I needed
> a predicate and zero register (since we don't have a zero-ing version
> of the insn) so I went with this.
>
> If I expand them since the resulting vector should be an SVE register
> converted from a scalar I'd just potentially re-introduce the problem
> again so I played it safe.
Yeah ok, it’s probably cleaner to leave it this way then.
Thanks,
Kyrill
>
> Thanks,
> Tamar
>
>>
>>> +
>>> +;; Shift an SVE vector left and insert a scalar into element 0 from a
>>> memory
>>> +;; load.
>>> +(define_insn "*aarch64_vec_shl_insert_from_load_<mode>"
>>> + [(set (match_operand:SVE_FULL 0 "register_operand")
>>> + (unspec:SVE_FULL
>>> + [(match_operand:SVE_FULL 1 "aarch64_simd_imm_zero")
>>> + (match_operand:<VEL> 2 "memory_operand")]
>>> + UNSPEC_INSR))]
>>> + "TARGET_SVE"
>>> + {@ [ cons: =0 , 1 , 2 ; attrs: movprfx ]
>>> + [ w , Dz , m; * ] ldr\t%<Vetype>0, %2
>>> + }
>>> + [(set_attr "type" "neon_move")]
>>> +)
>>
>> Same comment on movprfx being not needed.
>> Looks ok to me otherwise.
>> Thanks,
>> Kyrill
>>
>>
>>> +
>>> ;; -------------------------------------------------------------------------
>>> ;; ---- [INT] Linear series
>>> ;; -------------------------------------------------------------------------
>>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
>>> index
>> 619c2a6d2265aba2db837559646fbf51e571c00e..1a6737c74b4189afa29b
>> 902d815be3d50dfec084 100644
>>> --- a/gcc/config/aarch64/aarch64.cc
>>> +++ b/gcc/config/aarch64/aarch64.cc
>>> @@ -6887,29 +6887,6 @@ aarch64_stack_protect_canary_mem
>> (machine_mode mode, rtx decl_rtl,
>>> return gen_rtx_MEM (mode, force_reg (Pmode, addr));
>>> }
>>>
>>> -/* Emit a load/store from a subreg of SRC to a subreg of DEST.
>>> - The subregs have mode NEW_MODE. Use only for reg<->mem moves. */
>>> -void
>>> -aarch64_emit_load_store_through_mode (rtx dest, rtx src, machine_mode
>> new_mode)
>>> -{
>>> - gcc_assert ((MEM_P (dest) && register_operand (src, VOIDmode))
>>> - || (MEM_P (src) && register_operand (dest, VOIDmode)));
>>> - auto mode = GET_MODE (dest);
>>> - auto int_mode = aarch64_sve_int_mode (mode);
>>> - if (MEM_P (src))
>>> - {
>>> - rtx tmp = force_reg (new_mode, adjust_address (src, new_mode, 0));
>>> - tmp = force_lowpart_subreg (int_mode, tmp, new_mode);
>>> - emit_move_insn (dest, force_lowpart_subreg (mode, tmp, int_mode));
>>> - }
>>> - else
>>> - {
>>> - src = force_lowpart_subreg (int_mode, src, mode);
>>> - emit_move_insn (adjust_address (dest, new_mode, 0),
>>> - force_lowpart_subreg (new_mode, src, int_mode));
>>> - }
>>> -}
>>> -
>>> /* PRED is a predicate that is known to contain PTRUE.
>>> For 128-bit VLS loads/stores, emit LDR/STR.
>>> Else, emit an SVE predicated move from SRC to DEST. */
>>> @@ -26157,6 +26134,47 @@ aarch64_sve_expand_vector_init_subvector
>> (rtx target, rtx vals)
>>> return;
>>> }
>>>
>>> +/* Emit a load/store from a subreg of SRC to a subreg of DEST.
>>> + The subregs have mode NEW_MODE. Use only for reg<->mem moves. */
>>> +void
>>> +aarch64_emit_load_store_through_mode (rtx dest, rtx src, machine_mode
>> new_mode)
>>> +{
>>> + gcc_assert ((MEM_P (dest) && register_operand (src, VOIDmode))
>>> + || (MEM_P (src) && register_operand (dest, VOIDmode)));
>>> + auto mode = GET_MODE (dest);
>>> + auto int_mode = aarch64_sve_int_mode (mode);
>>> + rtx tmp_reg;
>>> + if (MEM_P (src))
>>> + {
>>> + rtx tmp = force_reg (new_mode, adjust_address (src, new_mode, 0));
>>> + if (!VECTOR_MODE_P (new_mode))
>>> + {
>>> + machine_mode full_mode = int_mode;
>>> + auto vmode = aarch64_classify_vector_mode (int_mode);
>>> + /* Partial vectors have to go through a full mode insert since we
>>> + don't support inserting an partial vectors. */
>>> + if (GET_MODE_INNER (int_mode) != new_mode || (vmode &
>> VEC_PARTIAL))
>>> + full_mode
>>> + = aarch64_full_sve_mode (as_a <scalar_mode> (new_mode)).require ();
>>> +
>>> + /* Create an SVE register with the top bits explicitly zero'd. */
>>> + tmp_reg = force_reg (full_mode, CONST0_RTX (full_mode));
>>> + emit_insr (tmp_reg, tmp);
>>> + if (full_mode != int_mode)
>>> + tmp_reg = force_lowpart_subreg (int_mode, tmp_reg, full_mode);
>>> + }
>>> + else
>>> + tmp_reg = force_lowpart_subreg (int_mode, tmp, new_mode);
>>> + emit_move_insn (dest, force_lowpart_subreg (mode, tmp_reg,
>> int_mode));
>>> + }
>>> + else
>>> + {
>>> + src = force_lowpart_subreg (int_mode, src, mode);
>>> + emit_move_insn (adjust_address (dest, new_mode, 0),
>>> + force_lowpart_subreg (new_mode, src, int_mode));
>>> + }
>>> +}
>>> +
>>> /* Check whether VALUE is a vector constant in which every element
>>> is either a power of 2 or a negated power of 2. If so, return
>>> a constant vector of log2s, and flip CODE between PLUS and MINUS
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/highway_run.c
>> b/gcc/testsuite/gcc.target/aarch64/sve/highway_run.c
>>> new file mode 100644
>>> index
>> 0000000000000000000000000000000000000000..b73fd51c63bed6f9b4e
>> 710546924ca60736d10e5
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/highway_run.c
>>> @@ -0,0 +1,55 @@
>>> +/* { dg-do run { target aarch64_sve_hw } } */
>>> +/* { dg-require-effective-target lp64 } */
>>> +/* { dg-options "-O2" } */
>>> +
>>> +#include <arm_sve.h>
>>> +
>>> +extern void abort (void) __attribute__ ((noreturn));
>>> +
>>> +volatile int cond = 1;
>>> +
>>> +int __attribute__ ((noipa))
>>> +a (void)
>>> +{
>>> + return cond;
>>> +}
>>> +
>>> +#define TEST_LOAD(TYPE, NAME, BITS) \
>>> + int __attribute__ ((noipa)) \
>>> + test_##NAME (void) \
>>> + { \
>>> + TYPE *g = __builtin_malloc (sizeof (TYPE)); \
>>> + int c = 0; \
>>> + if (!g) \
>>> + abort (); \
>>> + g[0] = 0; \
>>> + if (a ()) \
>>> + { \
>>> + g[0] = 1; \
>>> + c = 2; \
>>> + } \
>>> + svint##BITS##_t d \
>>> + = svld1_s##BITS (svptrue_pat_b##BITS (SV_VL1), g); \
>>> + svbool_t e = svcmpgt_s##BITS (svptrue_b##BITS (), d, \
>>> + svdup_n_s##BITS (0)); \
>>> + int f = svptest_any (svptrue_pat_b##BITS (SV_VL1), e); \
>>> + if (f && c != 2) \
>>> + abort (); \
>>> + return f; \
>>> + }
>>> +
>>> +TEST_LOAD (signed char, byte, 8)
>>> +TEST_LOAD (short, short, 16)
>>> +TEST_LOAD (int, int, 32)
>>> +TEST_LOAD (long, long, 64)
>>> +
>>> +int
>>> +main (void)
>>> +{
>>> + if (!test_byte ()
>>> + || !test_short ()
>>> + || !test_int ()
>>> + || !test_long ())
>>> + abort ();
>>> + return 0;
>>> +}
>>>
>>>
>>> --
>>> <rb20550.patch>
