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..fc98f47d746019959baa6a8aa29d6b996d8b4822
> 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.
That said, wouldn’t this be better as an insn_and_split so that the “and”
operation can be optimized and scheduled separately?
> +
> +;; 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..1a6737c74b4189afa29b902d815be3d50dfec084
> 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..b73fd51c63bed6f9b4e710546924ca60736d10e5
> --- /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>
