Christophe Lyon <christophe.l...@linaro.org> writes:
> On Wed, 9 Dec 2020 at 17:47, Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> > Hi,
>> >
>> > I've been working for a while on enabling auto-vectorization for ARM
>> > MVE, and I find it a bit awkward to keep things common with Neon as
>> > much as possible.
>> >
>> > I've just sent a few patches for logical operators
>> > (vand/vorr/veor/vbic), and I have a few more WIP patches where I
>> > struggle to avoid duplication.
>> >
>> > For example, vneg is supported in different modes by MVE and Neon:
>> > * Neon: VDQ and VH iterators: V8QI V16QI V4HI V8HI V2SI V4SI V4HF V8HF
>> > V2SF V4SF V2DI and V8HF V4HF
>> > * MVE: MVE_2 and MVE_0 iterators: V16QI V8HI V4SI and V8HF V4SF
>>
>> My hope behind the ARM_HAVE_<MODE>_<FOO> macros was that the common
>> (optab) define_expand could use those, with the most permissive iterator
>> necessary. We could stick on a "&& !TARGET_IWMMXT" for things that
>> aren't implemented for iwMMXt.
>>
>> The above combination seems like a natural fit for unmodified
>> VDQ with ARM_HAVE_<MODE>_ARITH. This would be similar to the
>> existing add<mode>3 pattern.
>>
>
> OK, so it looks like I should revert/fix my just-committed vand patch,
> and restore the unconditional definition of VDQ and use
> ARM_HAVE_<MODE>_ARITH for the expander?
That's one for the maintainers, the above is just my opinion.
I don't think a revert is necessary though. It looks like it
would be a small delta on top of the committed version.
>> > My 'vand' patch changes the definition of VDQ so that the relevant
>> > modes are enabled only when !TARGET_HAVE_MVE (V8QI, ...), and this
>> > helps writing a simpler expander.
>> >
>> > However, vneg is used by vshr (right-shifts by register are
>> > implemented as left-shift by negation of that register), so the
>> > expander uses something like:
>> >
>> > emit_insn (gen_neg<mode>2 (neg, operands[2]));
>> > if (TARGET_NEON)
>> > emit_insn (gen_ashl<mode>3_signed (operands[0], operands[1], neg));
>> > else
>> > emit_insn (gen_mve_vshlq_s<mode> (operands[0], operands[1], neg));
>> >
>> > which does not work if the iterator has conditional members: the
>> > 'else' part is still generated for <mode> unsupported by MVE.
>>
>> FWIW, I agree with Andre that it would be good to remove unnecessary
>> NEON/MVE differences like this.
>>
> OK thanks for the feedback, I'll update my other patches along these
> lines. Too bad this will delay auto-vectorization improvement more
> than I hoped :-(
Just to be sure, after seeing:
;; We use the same code as in neon.md (TODO: avoid this duplication).
in the vand patch:
I didn't mean above that we should consolidate MVE and NEON define_insns
that happen to be the same. I agree that should be at most a TODO (like
the one above).
It was more that it would be good to avoid having:
if (TARGET_NEON)
emit_insn (gen_neon_thing (…));
else
emit_insn (gen_mve_thing (…));
in cases where neon_thing and mve_thing have the same pattern.
Instead we can just have a single define_expand that generates
the common pattern for both architectures (if we don't already).
The common expand would work in a similar way to named optab patterns,
except that it's internal to the arm port.
FWIW, one hacky way of honouring the MVE intrinsic naming scheme
while using different names for the underlying .md patterns would
be to have:
const insn_code CODE_FOR_mve_<intrinsic_name> = CODE_FOR_<alias>;
in arm-builtins.c, perhaps wrapped in macros to autogenerate the
mode differences. Not elegant, for sure, but maybe it would be
useful for some things.
Thanks,
Richard
