On Thu, Jul 23, 2026 at 1:01 PM Richard Sandiford
<[email protected]> wrote:
>
> "Robin Dapp" <[email protected]> writes:
> >> Yeah, the aarch64 change looks good to me (but I shouldn't approve it).
> >>
> >> And sorry for the late reply. Since like you said the lower-subreg
> >> changes came from whack-a-mole, I kind-of needed to play the same game
> >> to review.
> >
> > No worries, the time frame allowed me to refine my year-old reassoc patch
> > that
> > is now supposed to pick up the crumbs the newly constrained (to
> > non-overflowing
> > types) backprop patch leaves behind. Luckily, it appears useful for both
> > the
> > new test case as well as the old one. Despite my concerns, bit-level
> > backprop
> > won't cause too much havoc on the IR and reassoc can still work. I'll send
> > a
> > respun "unsigned" backprop version at some point.
> >
> >> An aarch64 bootstrap & regression test succeeded for me except for:
> >>
> >> FAIL: gcc.target/aarch64/simd/fold_to_highpart_6.c
> >> check-function-bodies test_256b_type
> >>
> >> with:
> >>
> >> 1. the read_complex_part change (already approved)
> >> 2. the expemd.cc changes (already approved)
> >> 3. the interesting_mode_p change
> >> 4. the change below
> >> 5. the aarch64 change
> >>
> >> Do (3) and (4) work for you too, or do you still need some of the other
> >> lower-subreg changes from your patch? Could you give an example if so?
> >
> >> Could you list some of the testcases that fail for RISC-V without the
> >> emit_move_insn change? (Maybe you already did, sorry.)
> >>
> >> I won't be able to reply for a week or so. I'll have a look at
> >> fold_to_highpart_6.c when I get back.
> >
> > Not for the first time, messaging back and forth with you seems to make
> > issues disappear into thin air :)
> > I retested and indeed the emit_move_insn change looks redundant now. The
> > lower-subreg issues I only ever observed on aarch64, not on riscv. And your
> > lower-subreg change is at a far better spot than my distributed changes, I
> > just wonder why I didn't find that spot :/
> >
> > Thus, the attached v3 incorporates (1) - (5) and also includes the test case
> > of the second patch, just without the no-spill check.
> >
> > So far I only regtested it on riscv. aarch64, x86, and power10 are still
> > running. As you wanted to look into fold_to_highpart_6.c anyway, I'll
> > report back if there's any surprises from these runs.
>
> The fold_to_highpart_6.c failure was due to a difference in our aarch64.cc
> patches. I was testing with:
>
> if (!aarch64_sve_vg.is_constant ())
>
> removed entirely, rather than replaced with VECTOR_MODE_P. That had the
> effect of including OImode, CImode, and XImode. Given that:
>
> (a) after Jonathan's work, all real structures should now be using
> vector modes and
>
> (b) these *Imodes are presented as normal integer modes and so can
> be used for target-independent integer work (as here)
>
> I think your version is more correct. Of course, both versions are
> likely to have unfortunate corner cases, just as the status quo does.
>
> Now that the port defines sub-64-bit vector modes (a new thing), I think:
>
> > @@ -3060,20 +3060,17 @@ aarch64_regmode_natural_size (machine_mode mode)
> > /* The natural size for SVE data modes is one SVE data vector,
> > and similarly for predicates. We can't independently modify
> > anything smaller than that. */
> > - /* ??? For now, only do this for variable-width SVE registers.
> > - Doing it for constant-sized registers breaks lower-subreg.cc. */
> > - /* ??? And once that's fixed, we should probably have similar
> > - code for Advanced SIMD. */
> > - if (!aarch64_sve_vg.is_constant ())
> > - {
> > - /* REGMODE_NATURAL_SIZE influences general subreg validity rules,
> > - so we need to handle memory-only modes as well. */
> > + if (VECTOR_MODE_P (mode))
> > + {
> > unsigned int vec_flags = aarch64_classify_vector_memory_mode (mode);
> > if (vec_flags & VEC_SVE_PRED)
> > return BYTES_PER_SVE_PRED;
> > if (vec_flags & VEC_SVE_DATA)
> > return BYTES_PER_SVE_VECTOR;
> > + if (vec_flags & VEC_ADVSIMD)
> > + return exact_div (GET_MODE_SIZE (mode), aarch64_ldn_stn_vectors
> > (mode));
>
> ...this should be MAXed with UNITS_PER_WORD.
>
> The patch LGTM otherwise, but someone else should approve the target-
> independent and lower-subreg changes.
I'll rubber-stamp, if you reviewed those bits.
Richard.
>
> Thanks,
> Richard
>
>
> > }
> > +
> > return UNITS_PER_WORD;
> > }
> >
> > diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
> > index 431aaa1e761..4e5f011dc61 100644
> > --- a/gcc/config/riscv/riscv-v.cc
> > +++ b/gcc/config/riscv/riscv-v.cc
> > @@ -4011,14 +4011,10 @@ shuffle_even_odd_patterns (struct expand_vec_perm_d
> > *d)
> >
> > /* When the element width is smaller than the greatest ELEN, we can use
> > two
> > vnsrl instructions, each extracting the even/odd elements of one
> > source,
> > - and a vslideup instruction to merge them into one vector.
> > -
> > - PR target/124996: VLS mode subregs larger than what
> > - riscv_regmode_natural_size allows cause a memory roundtrip.
> > Therefore, for
> > - now, we only do this when the mode size is no greater than the
> > natural size
> > - of the register. Once this is fixed, the condition should be
> > replaced by
> > - the ELEN condition. */
> > - if (known_le (GET_MODE_SIZE (vmode), riscv_regmode_natural_size (vmode)))
> > + and a vslideup instruction to merge them into one vector. */
> > + unsigned int max_elen = TARGET_VECTOR_ELEN_64 ? 64 : 32;
> > + if (known_le (GET_MODE_SIZE (vmode), riscv_regmode_natural_size (vmode))
> > + && GET_MODE_BITSIZE (GET_MODE_INNER (vmode)) * 2 <= max_elen)
> > {
> > unsigned int elen = GET_MODE_BITSIZE (GET_MODE_INNER (vmode));
> > unsigned int elen2x = elen * 2;
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index d31d4a8aeab..26aacd0df36 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -13302,9 +13302,6 @@ riscv_regmode_natural_size (machine_mode mode)
> > /* The natural size for RVV data modes is one RVV data vector,
> > and similarly for predicates. We can't independently modify
> > anything smaller than that. */
> > - /* ??? For now, only do this for variable-width RVV registers.
> > - Doing it for constant-sized registers breaks lower-subreg.c. */
> > -
> > if (riscv_vector_mode_p (mode))
> > {
> > poly_uint64 size = GET_MODE_SIZE (mode);
> > @@ -13328,6 +13325,8 @@ riscv_regmode_natural_size (machine_mode mode)
> > return minimum size between vector register size and scalar
> > register size. */
> > return MIN (size.to_constant (), UNITS_PER_WORD);
> > + else
> > + return TARGET_MIN_VLEN / BITS_PER_WORD;
> > }
> > return UNITS_PER_WORD;
> > }
> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> > index da1b5b63287..c4db5ef10bd 100644
> > --- a/gcc/expmed.cc
> > +++ b/gcc/expmed.cc
> > @@ -858,12 +858,16 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
> > poly_uint64 bitnum,
> > valid for integral modes. */
> > opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
> > scalar_int_mode imode;
> > + bool need_stack_p = false;
> > if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
> > {
> > if (MEM_P (op0))
> > op0 = adjust_bitfield_address_size (op0, op0_mode.else_blk (),
> > 0, MEM_SIZE (op0));
> > - else if (!op0_mode.exists ())
> > + else if (!op0_mode.exists ()
> > + || maybe_lt
> > + ((unsigned) UNITS_PER_WORD,
> > + (poly_uint64) REGMODE_NATURAL_SIZE (GET_MODE (op0))))
> > {
> > if (ibitnum == 0
> > && known_eq (ibitsize, GET_MODE_BITSIZE (GET_MODE (op0)))
> > @@ -876,17 +880,29 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
> > poly_uint64 bitnum,
> > }
> > if (!fallback_p)
> > return false;
> > - rtx temp = assign_stack_temp (GET_MODE (op0),
> > - GET_MODE_SIZE (GET_MODE (op0)));
> > - emit_move_insn (temp, op0);
> > - store_bit_field_1 (temp, bitsize, bitnum, 0, 0, fieldmode, value,
> > - reverse, fallback_p, undefined_p);
> > - emit_move_insn (op0, temp);
> > - return true;
> > + need_stack_p = true;
> > }
> > else
> > op0 = gen_lowpart (op0_mode.require (), op0);
> > }
> > + else if (!MEM_P (op0)
> > + && maybe_lt ((unsigned) UNITS_PER_WORD,
> > + (poly_uint64) REGMODE_NATURAL_SIZE (GET_MODE (op0))))
> > + need_stack_p = true;
> > +
> > + /* With or without punning we might be faced with a mode that we cannot
> > + split into words. If so, spill OP0 to the stack and recurse.
> > + This happens at most once. */
> > + if (need_stack_p)
> > + {
> > + rtx temp = assign_stack_temp (GET_MODE (op0),
> > + GET_MODE_SIZE (GET_MODE (op0)));
> > + emit_move_insn (temp, op0);
> > + store_bit_field_1 (temp, bitsize, bitnum, 0, 0, fieldmode, value,
> > + reverse, fallback_p, undefined_p);
> > + emit_move_insn (op0, temp);
> > + return true;
> > + }
> >
> > return store_integral_bit_field (op0, op0_mode, ibitsize, ibitnum,
> > bitregion_start, bitregion_end,
> > @@ -1008,7 +1024,9 @@ store_integral_bit_field (rtx op0,
> > opt_scalar_int_mode op0_mode,
> > in BLKmode to handle unaligned memory references and to shift
> > the
> > last chunk right on big-endian machines if need be. */
> > rtx value_word
> > - = fieldmode == BLKmode
> > + = (fieldmode == BLKmode
> > + || maybe_lt ((unsigned) UNITS_PER_WORD,
> > + (poly_uint64) REGMODE_NATURAL_SIZE (value_mode)))
> > ? extract_bit_field (value, new_bitsize, wordnum *
> > BITS_PER_WORD,
> > 1, NULL_RTX, word_mode, word_mode, false,
> > NULL)
> > @@ -1834,12 +1852,16 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64
> > bitsize, poly_uint64 bitnum,
> > if we aren't. */
> > opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
> > scalar_int_mode imode;
> > + bool need_stack_p = false;
> > if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
> > {
> > if (MEM_P (op0))
> > op0 = adjust_bitfield_address_size (op0, op0_mode.else_blk (),
> > 0, MEM_SIZE (op0));
> > - else if (op0_mode.exists (&imode))
> > + else if (op0_mode.exists (&imode)
> > + && known_ge
> > + ((unsigned) UNITS_PER_WORD,
> > + (poly_uint64) REGMODE_NATURAL_SIZE (GET_MODE (op0))))
> > {
> > op0 = gen_lowpart (imode, op0);
> >
> > @@ -1849,12 +1871,19 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64
> > bitsize, poly_uint64 bitnum,
> > op0 = force_reg (imode, op0);
> > }
> > else
> > - {
> > - poly_int64 size = GET_MODE_SIZE (GET_MODE (op0));
> > - rtx mem = assign_stack_temp (GET_MODE (op0), size);
> > - emit_move_insn (mem, op0);
> > - op0 = adjust_bitfield_address_size (mem, BLKmode, 0, size);
> > - }
> > + need_stack_p = true;
> > + }
> > + else if (!MEM_P (op0)
> > + && maybe_lt ((unsigned) UNITS_PER_WORD,
> > + (poly_uint64) REGMODE_NATURAL_SIZE (GET_MODE (op0))))
> > + need_stack_p = true;
> > +
> > + if (need_stack_p)
> > + {
> > + poly_int64 size = GET_MODE_SIZE (GET_MODE (op0));
> > + rtx mem = assign_stack_temp (GET_MODE (op0), size);
> > + emit_move_insn (mem, op0);
> > + op0 = adjust_bitfield_address_size (mem, BLKmode, 0, size);
> > }
> >
> > /* ??? We currently assume TARGET is at least as big as BITSIZE.
> > diff --git a/gcc/expr.cc b/gcc/expr.cc
> > index f42aab21272..d39bb84d5da 100644
> > --- a/gcc/expr.cc
> > +++ b/gcc/expr.cc
> > @@ -4176,9 +4176,6 @@ read_complex_part (rtx cplx, bool imag_p)
> > imag_p ? GET_MODE_SIZE (imode) : 0);
> > if (ret)
> > return ret;
> > - else
> > - /* simplify_gen_subreg may fail for sub-word MEMs. */
> > - gcc_assert (MEM_P (cplx) && ibitsize < BITS_PER_WORD);
> > }
> >
> > return extract_bit_field (cplx, ibitsize, imag_p ? ibitsize : 0,
> > diff --git a/gcc/lower-subreg.cc b/gcc/lower-subreg.cc
> > index 5dee6a0b646..5033c6886c5 100644
> > --- a/gcc/lower-subreg.cc
> > +++ b/gcc/lower-subreg.cc
> > @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see
> > #include "memmodel.h"
> > #include "tm_p.h"
> > #include "expmed.h"
> > +#include "regs.h"
> > #include "insn-config.h"
> > #include "emit-rtl.h"
> > #include "recog.h"
> > @@ -113,6 +114,9 @@ interesting_mode_p (machine_mode mode, unsigned int
> > *bytes,
> > {
> > if (!GET_MODE_SIZE (mode).is_constant (bytes))
> > return false;
> > + if (maybe_lt ((unsigned) UNITS_PER_WORD,
> > + (poly_uint64) REGMODE_NATURAL_SIZE (mode)))
> > + return false;
> > *words = CEIL (*bytes, UNITS_PER_WORD);
> > return true;
> > }
> > @@ -302,7 +306,20 @@ static bool
> > simple_move_operand (rtx x)
> > {
> > if (GET_CODE (x) == SUBREG)
> > - x = SUBREG_REG (x);
> > + {
> > + /* Exclude subregs whose outer mode can be split into multiple words
> > + but whose inner mode cannot. Attempting to split such a subreg
> > + would mean trying to split the unsplittable inner register.
> > +
> > + If instead the subreg occupies a single word, we can keep it as-is,
> > + regardless of what the SUBREG_REG is. If the outer mode cannot be
> > + split then the subreg makes things no worse than they already are.
> > */
> > + unsigned int factor, size;
> > + if (interesting_mode_p (GET_MODE (x), &size, &factor) && factor > 1
> > + && !interesting_mode_p (GET_MODE (SUBREG_REG (x)), &size, &factor))
> > + return false;
> > + x = SUBREG_REG (x);
> > + }
> >
> > if (!OBJECT_P (x))
> > return false;
> > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/autovec/pr125390.c
> > b/gcc/testsuite/gcc.target/riscv/rvv/autovec/pr125390.c
> > new file mode 100644
> > index 00000000000..6ea9e509ba8
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/pr125390.c
> > @@ -0,0 +1,34 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gcv_zvl256b -mabi=lp64d -mrvv-vector-bits=zvl
> > -O2 --param=riscv-autovec-mode=RVVMF8QI -fno-vect-cost-model" } */
> > +
> > +int a, b[64], c[64];
> > +
> > +void
> > +foo (void)
> > +{
> > + for (unsigned e = 0; e < 8; e += 2)
> > + {
> > + a = c[e] ^ c[e + 1];
> > + b[e] = 0;
> > + }
> > +}
> > +
> > +void
> > +bar (void)
> > +{
> > + for (unsigned e = 0; e < 16; e += 2)
> > + {
> > + a = c[e] ^ c[e + 1];
> > + b[e] = 0;
> > + }
> > +}
> > +
> > +void
> > +baz (void)
> > +{
> > + for (unsigned e = 0; e < 32; e += 4)
> > + {
> > + a = c[e] ^ c[e + 1] ^ c[e + 2] ^ c[e + 3];
> > + b[e] = 0;
> > + }
> > +}
