"Robin Dapp" <[email protected]> writes:
>>> I suppose the question isn't so much "is this a tuple mode?" but more
>>> "does this vector occupy multiple registers?". REGMODE_NATURAL_SIZE
>>> seems like the right thing to test for that. The problem is that both
>>> AArch64 and RISC-V only define REGMODE_NATURAL_SIZE in the natural way
>>> for variable-length vectors:
>>>
>>> /* ??? For now, only do this for variable-width SVE registers.
>>> Doing it for constant-sized registers breaks lower-subreg.cc. */
>>>
>>> So I suppose we'd need to fix that first (which would be a good thing,
>>> but a bit of tangent).
>>
>> This has bitten me several times already but I never bothered to
>> investigate.
>> Agreed that it would be a good thing to fix. I'll have a look (but
>> not right
>> away).
>>
>>> Then related_vector_mode could be used to get the associated vector mode.
>
>
> I couldn't resist even though I have tons of other things to do :/
> But as one of my mid-term goals is to rework vec-vec extracts (for which we
> still rely on subregs surviving to the target), it seemed a reasonable fit.
>
> The attached patch/idea defines REGMODE_NATURAL_SIZE for VLS vectors as well
> and tries to handle the resulting fallout. At least for riscv, lower-subreg
> wasn't actually the problem but rather the arteries around emit_move,
> operand_force_subreg etc.
>
> Would an approach like the attached have any way forward? If so, a more
> thorough auditing of the various paths will be necessary. Obviously, the
> aarch64 changes are just ad-hoc. A previous version of it was
> bootstrapped/regtested as usual FWIW.
Thanks for trying this!
> diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
> index 431aaa1e761..3794f91c1cb 100644
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -4018,7 +4018,9 @@ shuffle_even_odd_patterns (struct expand_vec_perm_d *d)
> 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)))
> + 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;
FWIW, I suppose the comment above the check would need to be updated
as well. It sounds like the comment was anticipating that no
riscv_regmode_natural_size check would be needed after the fix,
but perhaps I misunderstood.
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index da1b5b63287..3e005958839 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -858,12 +858,15 @@ 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,
> + REGMODE_NATURAL_SIZE (GET_MODE (op0))))
> {
> if (ibitnum == 0
> && known_eq (ibitsize, GET_MODE_BITSIZE (GET_MODE (op0)))
> @@ -876,17 +879,31 @@ 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 (maybe_lt ((unsigned) UNITS_PER_WORD,
> + 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 +1025,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,
> + REGMODE_NATURAL_SIZE (value_mode)))
> ? extract_bit_field (value, new_bitsize, wordnum * BITS_PER_WORD,
> 1, NULL_RTX, word_mode, word_mode, false,
> NULL)
Long-term, I think both store_bit_field and extract_bit_field should
be taught to split into REGMODE_NATURAL_SIZE chunks of op0. The same
applies to users of operand_subword_force in general.
But clearly that would be a lot of work. And we'd need the value_mode
check in store_integral_bit_field even then.
So IMO the expmed changes above are OK.
> @@ -1834,12 +1853,15 @@ 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,
> + REGMODE_NATURAL_SIZE (GET_MODE (op0))))
> {
> op0 = gen_lowpart (imode, op0);
>
> @@ -1849,12 +1871,21 @@ 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 (maybe_lt ((unsigned) UNITS_PER_WORD,
> + 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.
I wonder how important it is in practice that we avoid forcing to memory
when the bitfield occupies the low word of a register. That would be
relatively complex to test, though, and I think any time spent on that
would be better spent on making extract_integral_bit_field handle
REGMODE_NATURAL_SIZE properly.
So IMO the extract_bit_field_1 change is OK as well.
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index de73215ccc6..5e0c659cad6 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -4173,8 +4173,20 @@ read_complex_part (rtx cplx, bool imag_p)
> if (ret)
> return ret;
> else
> - /* simplify_gen_subreg may fail for sub-word MEMs. */
> - gcc_assert (MEM_P (cplx) && ibitsize < BITS_PER_WORD);
> + {
> + /* Get the original reg we might be 'subreg'ing from.
> + If that has a vector mode we can fall back to
> + extract_bit_field.
> +
> + For scalar modes, simplify_gen_subreg may fail for
> + sub-word MEMs. */
> + rtx reg = cplx;
> + if (SUBREG_P (reg))
> + reg = SUBREG_REG (reg);
> + /* simplify_gen_subreg may fail for sub-word MEMs. */
> + gcc_assert (VECTOR_MODE_P (GET_MODE (reg))
> + || (MEM_P (cplx) && ibitsize < BITS_PER_WORD));
> + }
> }
>
> return extract_bit_field (cplx, ibitsize, imag_p ? ibitsize : 0,
I'd prefer to remove the assert here. I'm not sure what purpose
it serves. There are multiple valid reasons for simplify_gen_subreg
to return null and extract_bit_field is the correct fallback when
it does. extract_bit_field will itself assert if the combination
is really badly formed.
> @@ -4709,6 +4721,21 @@ emit_move_insn (rtx x, rtx y)
> y = y_inner;
> mode = GET_MODE (x_inner);
> }
> + /* If we don't have a move for MODE, emit_move_insn_1 will fall back to
> + emit_move_multi_word. The latter will call operand_subword_force,
> + which calls emit_move_insn again, without making progress.
> + Therefore, spill Y to memory and let the memory path below handle
> + it. */
> + else if (optab_handler (mov_optab, mode) == CODE_FOR_nothing
> + && y_inner && !MEM_P (x)
> + && maybe_gt (REGMODE_NATURAL_SIZE (GET_MODE (y_inner)),
> + (unsigned) UNITS_PER_WORD))
> + {
> + rtx mem = assign_stack_temp (GET_MODE (y_inner),
> + GET_MODE_SIZE (GET_MODE (y_inner)));
> + emit_move_insn (mem, y_inner);
> + y = adjust_address (mem, mode, 0);
> + }
>
> if (CONSTANT_P (y))
> {
Not sure I understand this one. Could you go into more detail?
Also, why is this keyed off y_inner being nonnull? Does the non-subreg
case work correctly? What about the case where y is a subreg but the
inner register is a different size (and thus y_inner is null)?
The "memory path below" just seems to "validize" the memories and
still falls through to emit_move_insn_1 in the end.
> diff --git a/gcc/lower-subreg.cc b/gcc/lower-subreg.cc
> index 5dee6a0b646..59b99c34554 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"
> @@ -1636,7 +1637,13 @@ decompose_multiword_subregs (bool decompose_copies)
> bitmap_clear (sub_blocks);
>
> EXECUTE_IF_SET_IN_BITMAP (decomposable_context, 0, regno, iter)
> - decompose_register (regno);
> + {
> + if (maybe_lt
> + ((unsigned) UNITS_PER_WORD,
> + REGMODE_NATURAL_SIZE (GET_MODE (regno_reg_rtx[regno]))))
> + continue;
> + decompose_register (regno);
> + }
>
> FOR_EACH_BB_FN (bb, cfun)
> {
It's not obvious to me how safe it is to pull out at this stage.
Did you consider making interesting_mode_p return false instead?
Richard
