"Robin Dapp" <[email protected]> writes:
>> Oops, I meant target-dependent of course.
>
> Grml, now I know why the issues disappeared... I accidentally
> re-introduced an older oversight in riscv's regmode_natural_size where I
> divided by 64 instead of by 8, leading to very limited coverage.
> I'll need at least two more hunks in expmed.cc and expr.cc both of which
> would be better off checking for a mov optab as well. Also, there is an
> ICE in simplify_subreg_concatn.
Could you give an example of something that needs the new changes?
Richard
> Rather than changing all callers of operand_subword_force, can we fix it
> in there directly? Even though they are mostly clustered, all of them
> would need to do the same thing anyway?
> I think by now (with the approved changes) extract_bit_field and
> store_bit_field can handle "word-unsplittable" vector modes already,
> either by extract/set or by spilling, so we could use those?
>
> With the changes below
>
> - aarch64/riscv hooks
> - accepted extract_bit_field/store_bit_field
> - accepted read_complex_part
> - your lower-subreg hunk
> - operand_subword_force using extract_bit_field
> - emit_move_multi_word using store_bit_field
>
> on riscv I only have one lower-subreg ICE left:
>
> FAIL: gcc.dg/pr44136.c (internal compiler error: in
> simplify_subreg_concatn, at lower-subreg.cc:678)
>
> Is that acceptable in order to avoid further whack-a-mole?
>
> ---
> gcc/config/aarch64/aarch64.cc | 16 ++++++++--------
> gcc/config/riscv/riscv-v.cc | 12 ++++--------
> gcc/config/riscv/riscv.cc | 5 ++---
> gcc/emit-rtl.cc | 9 +++++++++
> gcc/expmed.cc | 10 ++++++++--
> gcc/expr.cc | 19 +++++++++++++------
> gcc/lower-subreg.cc | 19 ++++++++++++++++++-
> 7 files changed, 62 insertions(+), 28 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 93c00c23a50..a2d67ea3d39 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -2995,20 +2995,20 @@ 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 MAX
> + (exact_div (GET_MODE_SIZE (mode),
> + aarch64_ldn_stn_vectors (mode)).to_constant (),
> + UNITS_PER_WORD);
> }
> +
> return UNITS_PER_WORD;
> }
>
> diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
> index 01960ff96e5..fd4fd89d05d 100644
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -4032,14 +4032,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 a401c0c7c93..a42fdc56e0e 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -13311,9 +13311,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);
> @@ -13337,6 +13334,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 / UNITS_PER_WORD;
> }
> return UNITS_PER_WORD;
> }
> diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
> index 4a23eaefe02..42d1b10090c 100644
> --- a/gcc/emit-rtl.cc
> +++ b/gcc/emit-rtl.cc
> @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3. If not see
> #include "cfgrtl.h"
> #include "tree-eh.h"
> #include "explow.h"
> +#include "expmed.h"
> #include "expr.h"
> #include "builtins.h"
> #include "rtl-iter.h"
> @@ -1827,6 +1828,14 @@ operand_subword_force (rtx op, poly_uint64 offset,
> machine_mode mode)
>
> if (mode != BLKmode && mode != VOIDmode)
> {
> + /* If the register cannot easily be split into words, let
> + extract_bit_field handle it. */
> + rtx tmp = SUBREG_P (op) ? SUBREG_REG (op) : op;
> + if (maybe_lt ((unsigned) UNITS_PER_WORD,
> + (poly_uint64) REGMODE_NATURAL_SIZE (GET_MODE (tmp))))
> + return extract_bit_field (tmp, BITS_PER_WORD,
> + offset * BITS_PER_WORD, 1, NULL_RTX,
> + word_mode, word_mode, false, NULL);
> /* If this is a register which cannot be accessed by words, copy it
> to a pseudo register. */
> if (REG_P (op))
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index b87d06bc9a4..9c719d8614a 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -863,7 +863,10 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
> poly_uint64 bitnum,
> 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)))
> @@ -1839,7 +1842,10 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
> poly_uint64 bitnum,
> 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);
>
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 210a7bc0888..7de94807550 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -4198,9 +4198,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,
> @@ -4567,11 +4564,21 @@ emit_move_multi_word (machine_mode mode, rtx x, rtx y)
> else if (ypart == 0)
> ypart = operand_subword_force (y, i, mode);
>
> - gcc_assert (xpart && ypart);
> + if (xpart == 0)
> + {
> + need_clobber = true;
> + store_bit_field (x, BITS_PER_WORD, i * BITS_PER_WORD, 0, 0,
> + word_mode, ypart, false, false);
> + last_insn = get_last_insn ();
> + }
> + else
> + {
> + gcc_assert (xpart && ypart);
>
> - need_clobber |= (GET_CODE (xpart) == SUBREG);
> + need_clobber |= (GET_CODE (xpart) == SUBREG);
>
> - last_insn = emit_move_insn (xpart, ypart);
> + last_insn = emit_move_insn (xpart, ypart);
> + }
> }
>
> seq = end_sequence ();
> 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;
