> -----Original Message----- > From: Gcc-patches <gcc-patches-boun...@gcc.gnu.org> On Behalf Of > Richard Sandiford via Gcc-patches > Sent: 10 December 2020 17:54 > To: Christophe Lyon <christophe.l...@linaro.org> > Cc: Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> > Subject: Re: RFC: ARM MVE and Neon auto-vectorization > > 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).
I agree. I think the patches as they are are a good move in the right direction and we can adjust them incrementally where it makes sense. Thanks, Kyrill > > 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
