"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.
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;
> + }
> +}
