> 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. Regards Robin >From 0cc7cf36c2ef7f5654c6cbe76ebd177db21ee7c9 Mon Sep 17 00:00:00 2001 From: Robin Dapp <[email protected]> Date: Wed, 24 Jun 2026 12:02:46 +0200 Subject: [PATCH v3] expand: Handle non-word addressable modes. In order to be able to set REGMODE_NATURAL_SIZE = "one vector register" for aarch64 and riscv, we need to make some adjustments to how we expand modes that cannot be split into words. In a few critical spots the assumption of "either we have a move or we can split into words" was implicitly hard coded. There is also the infinite-loop case where emit_move_multi_word, itself a fallback for "split moving" calls calls operand_subword_force which goes to force_reg again that uses emit_move_multi_word again. It turned into a small game of whack-a-mole and this patch takes the approach of just spilling to memory if REGMODE_NATURAL_SIZE > UNITS_PER_WORD. In lower subreg we just don't decompose a register if the same condition applies. Co-authored-by: Richard Sandiford <[email protected]> PR target/124996 PR middle-end/125390 gcc/ChangeLog: * config/aarch64/aarch64.cc (aarch64_regmode_natural_size): Return full vectors for all vector modes. * config/riscv/riscv-v.cc (shuffle_even_odd_patterns): Remove workaround. * config/riscv/riscv.cc (riscv_regmode_natural_size): Return full vector size. * expmed.cc (store_bit_field_1): Spill unsplittable modes to memory. (store_integral_bit_field): Defer unsplittable modes to extract_bit_field. (extract_bit_field_1): Spill if necessary. * expr.cc (read_complex_part): Adjust assert. * lower-subreg.cc (interesting_mode_p): Don't consider unsplittable modes as interesting. (simple_move_operand): Don't split subregs whose outer mode can be split but the inner mode cannot. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/autovec/pr125390.c: New test. --- gcc/config/aarch64/aarch64.cc | 13 ++-- gcc/config/riscv/riscv-v.cc | 12 ++-- gcc/config/riscv/riscv.cc | 5 +- gcc/expmed.cc | 61 ++++++++++++++----- gcc/expr.cc | 3 - gcc/lower-subreg.cc | 19 +++++- .../gcc.target/riscv/rvv/autovec/pr125390.c | 34 +++++++++++ 7 files changed, 108 insertions(+), 39 deletions(-) create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/pr125390.c diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index 78f1eae8336..99c839285a5 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -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)); } + 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; + } +} -- 2.54.0
