"Robin Dapp" <[email protected]> writes:
> This is the second patch now. The "tuple/multi-vector-reg -> vector"
> functionality depends on REGMODE_NATURAL_SIZE now but works as before apart
> from that. Unfortunately, the diff is a bit garbled now, as I moved things
> around. Sorry about that.
>
> Bootstrapped and regtested on x86, power10, and aarch64.
> Regtested on riscv64.
>
> Regards
> Robin
>
>
> [PATCH] expand: Improve tuple-extraction handling.
>
> After the REGMODE_NATURAL_SIZE changes have landed, this is another small
> improvement for PR125390 which also fixes aarch64's struct_3_256.c
> regression.
> Originally a riscv-specific issue and ICE, I noticed poor codegen when we
> extract values from a tuple type. In the optimized dump we have:
>
> vect_array.9 = .MASK_LEN_LOAD_LANES (&c, 32B, { -1, -1, -1, -1 }, _44(D),
> 4, 0);
> _48 = BIT_FIELD_REF <vect_array.9[0], 32, 96>;
> _50 = BIT_FIELD_REF <vect_array.9[1], 32, 96>;
> f_13 = _48 ^ _50;
> a = f_13;
>
> and _48 as well as _50 spill to memory instead of extracting directly.
>
> We end up with
>
> (subreg:RVVMF2SI (reg:RVVMF2x2SI 138 [ vect_array.9 ]) 0)
>
> which, as we pun the with a large integer mode, involves OImode
> that we cannot "move". We might be able to work around that in the
> target in unintuitive ways :) and we could also offer vec_extract for
> all tuple modes. However, I figured it would be much easier if expand
> already handled the tuple -> vector part for us so just a vector extract
> is left for the target to do.
>
> The patch adds multi-vector-reg -> vector functionality as a separate
> function and also moves vector-vector, as well as vector-element extract into
> new functions.
>
> I couldn't resist refactoring GET_MODE (op0) to outermode throughout the
> functions. If that obstructs reviewing, I'm happy to revert that part.
I don't particularly mind, but I think it would be good to avoid the
shadowing with:
{
class expand_operand ops[3];
machine_mode outermode = new_mode;
machine_mode innermode = tmode;
enum insn_code icode
= convert_optab_handler (vec_extract_optab, outermode, innermode);
especially since the modes can be different.
TBH "outermode" doesn't seem very appropriate in the vector_from_vector
context. Looking at the history, it seems that the outermode/innermode
names were copied over from the extract code, where the innermode really
is the GET_MODE_INNER of the outermode.
That's just surface stuff though.
> PR middle-end/125390
>
> gcc/ChangeLog:
>
> * expmed.cc (extract_bit_field_1): Call new functions.
> (extract_vector_from_vector): New function.
> (extract_element_from_vector): Likewise.
> (extract_vector_from_multireg): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/rvv/autovec/pr125390.c: New test.
> ---
> gcc/expmed.cc | 212 +++++++++++++-----
> .../gcc.target/riscv/rvv/autovec/pr125390.c | 36 +++
> 2 files changed, 194 insertions(+), 54 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/pr125390.c
>
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index c4db5ef10bd..ae6b0fd0ecf 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -1680,65 +1680,34 @@ extract_bit_field_as_subreg (machine_mode mode, rtx
> op0,
> return NULL_RTX;
> }
>
> -/* A subroutine of extract_bit_field, with the same arguments.
> - If UNSIGNEDP is -1, the result need not be sign or zero extended.
> - If FALLBACK_P is true, fall back to extract_fixed_bit_field
> - if we can find no other means of implementing the operation.
> - if FALLBACK_P is false, return NULL instead. */
> +/* Try to extract a sub-vector of mode TMODE (and same BITSIZE)
> + from the vector OP0 at position BITNUM. Lowpart the result of
> + mode MODE with TMODE if necessary. Return the extracted value
> + or NULL_RTX if we failed. */
Not sure about "lowpart" as a verb, so how about:
/* Try to extract a subvector of mode TMODE (which has BITSIZE bits)
from vector OP0, starting at bit BITNUM, then extract the MODE lowpart of
the subvector. Return the result on success or NULL_RTX on failure. */
Feel free to reword.
> static rtx
> -extract_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
> - int unsignedp, rtx target, machine_mode mode,
> - machine_mode tmode, bool reverse, bool fallback_p,
> - rtx *alt_rtl)
> +extract_vector_from_vector (rtx op0, rtx target, machine_mode mode,
> + machine_mode tmode, poly_uint64 bitsize,
> + poly_uint64 bitnum, rtx *alt_rtl)
> {
> - rtx op0 = str_rtx;
> - machine_mode mode1;
> -
> - if (tmode == VOIDmode)
> - tmode = mode;
> -
> - while (GET_CODE (op0) == SUBREG)
> - {
> - bitnum += SUBREG_BYTE (op0) * BITS_PER_UNIT;
> - op0 = SUBREG_REG (op0);
> - }
> -
> - /* If we have an out-of-bounds access to a register, just return an
> - uninitialized register of the required mode. This can occur if the
> - source code contains an out-of-bounds access to a small array. */
> - if (REG_P (op0) && known_ge (bitnum, GET_MODE_BITSIZE (GET_MODE (op0))))
> - return gen_reg_rtx (tmode);
> -
> - if (REG_P (op0)
> - && mode == GET_MODE (op0)
> - && known_eq (bitnum, 0U)
> - && known_eq (bitsize, GET_MODE_BITSIZE (GET_MODE (op0))))
> - {
> - if (reverse)
> - op0 = flip_storage_order (mode, op0);
> - /* We're trying to extract a full register from itself. */
> - return op0;
> - }
> -
> - /* First try to check for vector from vector extractions. */
> - if (VECTOR_MODE_P (GET_MODE (op0))
> + machine_mode outermode = GET_MODE (op0);
> + if (VECTOR_MODE_P (outermode)
> && !MEM_P (op0)
> && VECTOR_MODE_P (tmode)
> && known_eq (bitsize, GET_MODE_PRECISION (tmode))
> && maybe_gt (GET_MODE_SIZE (GET_MODE (op0)), GET_MODE_SIZE (tmode)))
> {
> - machine_mode new_mode = GET_MODE (op0);
> + machine_mode new_mode = outermode;
> if (GET_MODE_INNER (new_mode) != GET_MODE_INNER (tmode))
> {
> scalar_mode inner_mode = GET_MODE_INNER (tmode);
> poly_uint64 nunits;
> - if (!multiple_p (GET_MODE_BITSIZE (GET_MODE (op0)),
> + if (!multiple_p (GET_MODE_BITSIZE (outermode),
> GET_MODE_UNIT_BITSIZE (tmode), &nunits)
> || !related_vector_mode (tmode, inner_mode,
> nunits).exists (&new_mode)
> || maybe_ne (GET_MODE_SIZE (new_mode),
> - GET_MODE_SIZE (GET_MODE (op0))))
> + GET_MODE_SIZE (outermode)))
> new_mode = VOIDmode;
> }
> poly_uint64 pos;
The:
new_mode = VOIDmode;
}
poly_uint64 pos;
if (new_mode != VOIDmode
dance isn't really necessary now. We could just return early instead.
That said, I suppose keeping the current structure is more future-proof
against other cases' being added later.
There again, embracing a single check and returning early would mean
that we could avoid new_mode altogether and simply update outermode
(or whatever we call it). That would also solve the shadowing issue.
> @@ -1753,11 +1722,12 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64
> bitsize, poly_uint64 bitnum,
> enum insn_code icode
> = convert_optab_handler (vec_extract_optab, outermode, innermode);
>
> - if (new_mode != GET_MODE (op0))
> - op0 = gen_lowpart (new_mode, op0);
> + rtx from = op0;
> + if (new_mode != GET_MODE (from))
> + from = gen_lowpart (new_mode, op0);
> create_output_operand (&ops[0], target, innermode);
> ops[0].target = 1;
> - create_input_operand (&ops[1], op0, outermode);
> + create_input_operand (&ops[1], from, GET_MODE (from));
> create_integer_operand (&ops[2], pos);
> if (maybe_expand_insn (icode, 3, ops))
> {
> @@ -1771,10 +1741,23 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64
> bitsize, poly_uint64 bitnum,
> }
> }
>
> + return NULL_RTX;
> +}
> +
> +/* Extract an element of OP0 of size BITSIZE, starting at bit position
> BITNUM.
> + Pun the result with mode TMODE if necessary. Return the extracted element
> + if successful or NULL_RTX otherwise. */
> +
> +static rtx
> +extract_element_from_vector (rtx op0, rtx target, machine_mode mode,
> + machine_mode tmode, poly_uint64 bitsize,
> + poly_uint64 bitnum, rtx *alt_rtl)
> +{
> /* See if we can get a better vector mode before extracting. */
> - if (VECTOR_MODE_P (GET_MODE (op0))
> + machine_mode outermode = GET_MODE (op0);
> + if (VECTOR_MODE_P (outermode)
> && !MEM_P (op0)
> - && GET_MODE_INNER (GET_MODE (op0)) != tmode)
> + && GET_MODE_INNER (outermode) != tmode)
> {
> machine_mode new_mode;
>
> @@ -1792,21 +1775,18 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64
> bitsize, poly_uint64 bitnum,
> new_mode = MIN_MODE_VECTOR_INT;
>
> FOR_EACH_MODE_FROM (new_mode, new_mode)
> - if (known_eq (GET_MODE_SIZE (new_mode), GET_MODE_SIZE (GET_MODE (op0)))
> + if (known_eq (GET_MODE_SIZE (new_mode), GET_MODE_SIZE (outermode))
> && known_eq (GET_MODE_UNIT_SIZE (new_mode), GET_MODE_SIZE (tmode))
> && known_eq (bitsize, GET_MODE_UNIT_PRECISION (new_mode))
> && multiple_p (bitnum, GET_MODE_UNIT_PRECISION (new_mode))
> && targetm.vector_mode_supported_p (new_mode)
> - && targetm.modes_tieable_p (GET_MODE (op0), new_mode))
> + && targetm.modes_tieable_p (outermode, new_mode))
> break;
> if (new_mode != VOIDmode)
> op0 = gen_lowpart (new_mode, op0);
> }
>
> - /* Use vec_extract patterns for extracting parts of vectors whenever
> - available. If that fails, see whether the current modes and bitregion
> - give a natural subreg. */
> - machine_mode outermode = GET_MODE (op0);
> + outermode = GET_MODE (op0);
> if (VECTOR_MODE_P (outermode) && !MEM_P (op0))
> {
> scalar_mode innermode = GET_MODE_INNER (outermode);
> @@ -1848,6 +1828,130 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64
> bitsize, poly_uint64 bitnum,
> }
> }
>
> + return NULL_RTX;
> +}
> +
> +/* Get a vector from a multi-vector-register mode (like a tuple mode).
> + If OP0 has such a mode and TMODE as well as BITSIZE span a full
Do you mean fit within a single register, rather than span a full subvector?
ISTM that we're choosing the subvector to suit our purposes, based on TMODE
and BITSIZE, rather than TMODE and BITSIZE being constrained by an already-
known subvector. If so...
> + sub-vector, force it into a register of an appropriately-sized
> + vector mode. Return the vector if successful or NULL_RTX otherwise. */
...how about:
/* Check whether:
(a) OP0 is a vector that spans multiple registers (which might
mean that it is a tuple mode) and
(b) a sequence of BITSIZE bits, starting at bit *BITNUM, fits within
a single vector register.
Return the vector register if so and update *BITNUM so that it
is relative to the returned register. Return NULL_RTX otherwise. */
Again feel free to reword.
> +
> +static rtx
> +extract_vector_from_multireg (rtx op0, machine_mode tmode,
> + poly_uint64 bitsize, poly_uint64 *bitnum)
> +{
> + machine_mode outermode = GET_MODE (op0);
> + poly_uint64 outer_natural_sz = REGMODE_NATURAL_SIZE (outermode);
> + HOST_WIDE_INT vec_idx;
> + poly_uint64 off;
> + if (VECTOR_MODE_P (outermode)
> + && !MEM_P (op0)
> + && known_gt (GET_MODE_SIZE (outermode), outer_natural_sz)
> + && known_eq (bitsize, GET_MODE_PRECISION (tmode))
> + && known_le (GET_MODE_SIZE (tmode), outer_natural_sz)
> + && can_div_trunc_p (*bitnum, outer_natural_sz * BITS_PER_UNIT,
> + &vec_idx, &off)
> + && known_le (off + bitsize, outer_natural_sz * BITS_PER_UNIT))
> + {
> + /* Get a single-vector mode from the outer multi-vector mode.
> + If it exists and we can pun the outer mode with a subreg,
> + directly force it into a register. The subreg should be
> + gone by then. */
Why do we need to force it into a register? That can be counterproductive,
since it means that both the subvector and the tuple are live at the same
time. I realise we probably expect this to be optimised away later,
but still, it feels a bit premature.
I suppose most of extract_bit_field_1 is written assuming that:
while (GET_CODE (op0) == SUBREG)
{
bitnum += SUBREG_BYTE (op0) * BITS_PER_UNIT;
op0 = SUBREG_REG (op0);
}
has dealt with the subreg case, but given this change, I think it
would be better to make the code more robust against SUBREGs instead.
The while loop above managed to deal with subregs without emitting code,
but doing that here would undo the optimisation.
Or is forcing to a REG necessary for precisely that reason? Does keeping
the SUBREG lead to inefficiencies later?
> + scalar_mode elmode = GET_MODE_INNER (outermode);
> + poly_uint64 nelts;
> + multiple_p (outer_natural_sz, GET_MODE_SIZE (elmode), &nelts);
> + opt_machine_mode vmode
> + = related_vector_mode (outermode, GET_MODE_INNER (outermode), nelts);
> +
> + machine_mode vmode_onevec;
> + rtx subreg;
> + if (vmode.exists (&vmode_onevec)
> + && (subreg = simplify_gen_subreg
> + (vmode_onevec, op0, outermode,
> + vec_idx * GET_MODE_SIZE (vmode_onevec))))
> + {
> + op0 = force_reg (vmode_onevec, subreg);
> + *bitnum = off;
> + return op0;
> + }
> + }
> +
> + return NULL_RTX;
> +}
> +
> +/* A subroutine of extract_bit_field, with the same arguments.
> + If UNSIGNEDP is -1, the result need not be sign or zero extended.
> + If FALLBACK_P is true, fall back to extract_fixed_bit_field
> + if we can find no other means of implementing the operation.
> + if FALLBACK_P is false, return NULL instead. */
> +
> +static rtx
> +extract_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
> + int unsignedp, rtx target, machine_mode mode,
> + machine_mode tmode, bool reverse, bool fallback_p,
> + rtx *alt_rtl)
> +{
> + rtx op0 = str_rtx;
> + machine_mode mode1;
> +
> + if (tmode == VOIDmode)
> + tmode = mode;
> +
> + while (GET_CODE (op0) == SUBREG)
> + {
> + bitnum += SUBREG_BYTE (op0) * BITS_PER_UNIT;
> + op0 = SUBREG_REG (op0);
> + }
> +
> + /* If we have an out-of-bounds access to a register, just return an
> + uninitialized register of the required mode. This can occur if the
> + source code contains an out-of-bounds access to a small array. */
> + if (REG_P (op0) && known_ge (bitnum, GET_MODE_BITSIZE (GET_MODE (op0))))
> + return gen_reg_rtx (tmode);
> +
> + if (REG_P (op0)
> + && mode == GET_MODE (op0)
> + && known_eq (bitnum, 0U)
> + && known_eq (bitsize, GET_MODE_BITSIZE (GET_MODE (op0))))
> + {
> + if (reverse)
> + op0 = flip_storage_order (mode, op0);
> + /* We're trying to extract a full register from itself. */
> + return op0;
> + }
> +
> + /* First try to check for vector from vector extractions. */
> + rtx res = extract_vector_from_vector (op0, target, mode, tmode, bitsize,
> + bitnum, alt_rtl);
> + if (res)
> + return res;
> +
> + /* Use vec_extract patterns for extracting parts of vectors whenever
> + available. If that fails, see whether the current modes and bitregion
> + give a natural subreg. */
> + res = extract_element_from_vector (op0, target, mode, tmode, bitsize,
> bitnum,
> + alt_rtl);
> + if (res)
> + return res;
> +
> + /* If this mode occupies multiple registers and we're extracting
> + a single one, we can usually get to that vector directly
> + without actual extraction.
> + This helps targets see through multi-vector modes. */
> + poly_uint64 bitnum_new = bitnum;
> + res = extract_vector_from_multireg (op0, tmode, bitsize, &bitnum_new);
> + if (res)
> + {
> + rtx el;
> + el = extract_element_from_vector (res, target, mode, tmode, bitsize,
> + bitnum_new, alt_rtl);
> + if (el)
> + return el;
Could we handle vector_from_vector here too, to reduce the number of patterns
that are needed?
Otherwise it looks good.
Thanks,
Richard
> +
> + op0 = res;
> + bitnum = bitnum_new;
> + }
> +
> /* Make sure we are playing with integral modes. Pun with subregs
> if we aren't. */
> opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
> 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..536c71c174a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/pr125390.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gcv_zvl256b -mabi=lp64d -mrvv-vector-bits=zvl
> -O2 --param=riscv-autovec-mode=RVVMF8QI -fno-vect-cost-model
> -fdump-rtl-expand" } */
> +
> +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;
> + }
> +}
> +
> +/* { dg-final { scan-rtl-dump-not "virtual-stack-vars" "expand" } } */
