Richard Sandiford <[email protected]> writes:
> "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.
Oops, I meant target-dependent of course.
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;
>> + }
>> +}
