Re: RFC/A: Add a targetm.vectorize.related_mode hook

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Oct 30, 2019 at 10:43 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> The series posted so far now shows how the hook would be used in practice.
> Just wanted to follow up on some points here:
>
> Richard Sandiford <richard.sandiford@arm.com> writes:
> > Richard Biener <richard.guenther@gmail.com> writes:
> >> On Wed, Oct 23, 2019 at 2:12 PM Richard Sandiford
> >> <richard.sandiford@arm.com> wrote:
> >>>
> >>> Richard Biener <richard.guenther@gmail.com> writes:
> >>> > On Wed, Oct 23, 2019 at 1:51 PM Richard Sandiford
> >>> > <richard.sandiford@arm.com> wrote:
> >>> >>
> >>> >> Richard Biener <richard.guenther@gmail.com> writes:
> >>> >> > On Wed, Oct 23, 2019 at 1:00 PM Richard Sandiford
> >>> >> > <richard.sandiford@arm.com> wrote:
> >>> >> >>
> >>> >> >> This patch is the first of a series that tries to remove two
> >>> >> >> assumptions:
> >>> >> >>
> >>> >> >> (1) that all vectors involved in vectorisation must be the same size
> >>> >> >>
> >>> >> >> (2) that there is only one vector mode for a given element mode and
> >>> >> >>     number of elements
> >>> >> >>
> >>> >> >> Relaxing (1) helps with targets that support multiple vector sizes or
> >>> >> >> that require the number of elements to stay the same.  E.g. if we're
> >>> >> >> vectorising code that operates on narrow and wide elements, and the
> >>> >> >> narrow elements use 64-bit vectors, then on AArch64 it would normally
> >>> >> >> be better to use 128-bit vectors rather than pairs of 64-bit vectors
> >>> >> >> for the wide elements.
> >>> >> >>
> >>> >> >> Relaxing (2) makes it possible for -msve-vector-bits=128 to preoduce
> >>> >> >> fixed-length code for SVE.  It also allows unpacked/half-size SVE
> >>> >> >> vectors to work with -msve-vector-bits=256.
> >>> >> >>
> >>> >> >> The patch adds a new hook that targets can use to control how we
> >>> >> >> move from one vector mode to another.  The hook takes a starting vector
> >>> >> >> mode, a new element mode, and (optionally) a new number of elements.
> >>> >> >> The flexibility needed for (1) comes in when the number of elements
> >>> >> >> isn't specified.
> >>> >> >>
> >>> >> >> All callers in this patch specify the number of elements, but a later
> >>> >> >> vectoriser patch doesn't.  I won't be posting the vectoriser patch
> >>> >> >> for a few days, hence the RFC/A tag.
> >>> >> >>
> >>> >> >> Tested individually on aarch64-linux-gnu and as a series on
> >>> >> >> x86_64-linux-gnu.  OK to install?  Or if not yet, does the idea
> >>> >> >> look OK?
> >>> >> >
> >>> >> > In isolation the idea looks good but maybe a bit limited?  I see
> >>> >> > how it works for the same-size case but if you consider x86
> >>> >> > where we have SSE, AVX256 and AVX512 what would it return
> >>> >> > for related_vector_mode (V4SImode, SImode, 0)?  Or is this
> >>> >> > kind of query not intended (where the component modes match
> >>> >> > but nunits is zero)?
> >>> >>
> >>> >> In that case we'd normally get V4SImode back.  It's an allowed
> >>> >> combination, but not very useful.
> >>> >>
> >>> >> > How do you get from SVE fixed 128bit to NEON fixed 128bit then?  Or is
> >>> >> > it just used to stay in the same register set for different component
> >>> >> > modes?
> >>> >>
> >>> >> Yeah, the idea is to use the original vector mode as essentially
> >>> >> a base architecture.
> >>> >>
> >>> >> The follow-on patches replace vec_info::vector_size with
> >>> >> vec_info::vector_mode and targetm.vectorize.autovectorize_vector_sizes
> >>> >> with targetm.vectorize.autovectorize_vector_modes.  These are the
> >>> >> starting modes that would be passed to the hook in the nunits==0 case.
> >>> >>
> >>> >> E.g. for Advanced SIMD on AArch64, it would make more sense for
> >>> >> related_mode (V4HImode, SImode, 0) to be V4SImode rather than V2SImode.
> >>> >> I think things would work in a similar way for the x86_64 vector archs.
> >>> >>
> >>> >> For SVE we'd add both VNx16QImode (the SVE mode) and V16QImode (the
> >>> >> Advanced SIMD mode) to autovectorize_vector_modes, even though they
> >>> >> happen to be the same size for 128-bit SVE.  We can then compare
> >>> >> 128-bit SVE with 128-bit Advanced SIMD, with related_mode ensuring
> >>> >> that we consistently use all-SVE modes or all-Advanced SIMD modes
> >>> >> for each attempt.
> >>> >>
> >>> >> The plan for SVE is to add 4(!) modes to autovectorize_vector_modes:
> >>> >>
> >>> >> - VNx16QImode (full vector)
> >>> >> - VNx8QImode (half vector)
> >>> >> - VNx4QImode (quarter vector)
> >>> >> - VNx2QImode (eighth vector)
> >>> >>
> >>> >> and then pick the one with the lowest cost.  related_mode would
> >>> >> keep the number of units the same for nunits==0, within the limit
> >>> >> of the vector size.  E.g.:
> >>> >>
> >>> >> - related_mode (VNx16QImode, HImode, 0) == VNx8HImode (full vector)
> >>> >> - related_mode (VNx8QImode, HImode, 0) == VNx8HImode (full vector)
> >>> >> - related_mode (VNx4QImode, HImode, 0) == VNx4HImode (half vector)
> >>> >> - related_mode (VNx2QImode, HImode, 0) == VNx2HImode (quarter vector)
> >>> >>
> >>> >> and:
> >>> >>
> >>> >> - related_mode (VNx16QImode, SImode, 0) == VNx4SImode (full vector)
> >>> >> - related_mode (VNx8QImode, SImode, 0) == VNx4SImode (full vector)
> >>> >> - related_mode (VNx4QImode, SImode, 0) == VNx4SImode (full vector)
> >>> >> - related_mode (VNx2QImode, SImode, 0) == VNx2SImode (half vector)
> >>> >>
> >>> >> So when operating on multiple element sizes, the tradeoff is between
> >>> >> trying to make full use of the vector size (higher base nunits) vs.
> >>> >> trying to remove packs and unpacks between multiple vector copies
> >>> >> (lower base nunits).  The latter is useful because extending within
> >>> >> a vector is an in-lane rather than cross-lane operation and truncating
> >>> >> within a vector is a no-op.
> >>> >>
> >>> >> With a couple of tweaks, we seem to do a good job of guessing which
> >>> >> version has the lowest cost, at least for the simple cases I've tried
> >>> >> so far.
> >>> >>
> >>> >> Obviously there's going to be a bit of a compile-time cost
> >>> >> for SVE targets, but I think it's worth paying for.
> >>> >
> >>> > I would guess that immediate benefit could be seen with
> >>> > basic-block vectorization which simply fails when conversions
> >>> > are involved.  x86_64 should now always support V4SImode
> >>> > and V2SImode so eventually a testcase can be crafted for that
> >>> > as well.
> >>>
> >>> I'd hoped so too, but the problem is that if the cost saving is good
> >>> enough, BB vectorisation simply stops at the conversion frontiers and
> >>> vectorises the rest, rather than considering other vector mode
> >>> combinations that might be able to do more.
> >>
> >> Sure, but when SLP build fails because it thinks it needs to unroll
> >> it could ask for a vector type with the same number of lanes
> >
> > Do you mean for loop or BB vectorisation?  For loop vectorisation
> > the outer loop iterating over vector sizes/modes works fine: if we
> > need to unroll beyond the maximum VF, we'll just retry vectorisation
> > with a different vector size/mode combination, with the narrowest
> > element having smaller vectors.
> >
> > But yeah, I guess if get_vectype_for_scalar_type returns a vector
> > with too many units for BB vectorisation, we could try asking for
> > a different type with the "right" number of units, rather than
> > failing and falling back to the next iteration of the outer loop.
> > I'll give that a go.
>
> This is 16/n: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02063.html
> It was easier than I'd expected and cleans up some nasty codegen for
> mixed-size SLP on AArch64, so thanks :-)
>
> On iterating over VFs:
>
> >> (that's probably what we should do from the start - get same number
> >> of lane vector types in BB vectorization).
> >
> > It's still useful to have different numbers of lanes for both loop
> > and BB vectorisation.  E.g. if we're applying SLP to an operation on
> > 8 ints and 8 shorts, it's still better to mix V4SI and V8HI for 128-bit
> > vector archs.
> >
> >> It's when you introduce multiple sizes then the outer loop over all
> >> sizes comparing costs becomes somewhat obsolete...  this outer
> >> loop should then instead compare different VFs (which also means
> >> possible extra unrolling beyond the vector size need?).
> >
> > This is effectively what we're doing for the SVE arrangement above.
> > But replacing targetm.vectorize.autovectorize_vector_sizes with
> > targetm.vectorize.autovectorize_vector_modes means that we can
> > also try multiple vector archs with the same VF (e.g. 128-bit SVE
> > vs. 128-bit Advanced SIMD).
>
> See 12/n: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01831.html
> for how this iteration works in practice.  The AArch64 port specifies
> four ways of picking vector types, depending on which elements (if any)
> use 64 bits instead of 128 bits.  Each of these approaches then determines
> a VF, based on the element types that are actually used.  Sometimes we
> can end up trying the same VF and vector types multiple times, but that's
> easy to fix -- will post a patch soon as preparation for picking the
> "best" size.
>
> I think iterating on these approaches is better than iterating directly
> on candidate VFs for the reason mentioned above: full-vector 128-bit SVE
> and full-vector 128-bit Advanced SIMD both provide the same VF, but we
> want to try them both.
>
> And like you say, if base vector mode M1 gives a VF of X and base vector
> mode M2 gives a VF of X/2, and if 2*M2 appears cheaper than M1, we could
> in future have the option of doubling the VF for M2 artificially as a
> way of getting interleave-based unrolling.  So in that case we'd have
> two different approaches for vectorising at VF X even if we stick to the
> same vector architecture.
>
> So I don't think starting with the VF and calculating other things from
> that is intrinsically better.  We'd then have to have a subloop that
> iterates over Advanced SIMD vs. SVE, and potentially a further subloop
> that iterates over unroll factors.

OK, fair enough.

The patch is OK.

Thanks,
Richard.

> Thanks,
> Richard