RFC: ARM MVE and Neon auto-vectorization

[Andre Vieira (lists)]

On 08/12/2020 13:50, Christophe Lyon via Gcc-patches wrote:
> Hi,
>
>
> 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.
>
> So.... I guess my question is:  do we want to enforce implementation
> of Neon / MVE common parts? There are already lots of partly
> overlapping/duplicate iterators. I have tried to split iterators into
> eg VDQ_COMMON_TO_NEON_AND_MVE and VDQ_NEON_ONLY but this means we have
> to basically duplicate the expanders which defeats the point...
Ideally I think we'd want a minimal number iterators and defines, which 
was the idea behind the conditional iterators disabling 64-bit modes for 
MVE.

Obviously that then breaks the code above. For this specific case I 
would suggest unifying define_insns ashl<mode>3_{signed,unsigned} and 
mve_vshlq_<supf><mode>, they are very much the same patterns, I also 
don't understand why ahsl's signed and unsigned are separate. For 
instance create a 'ashl3_<supf>_<mode>' or something like that, and make 
sure the calls to gen_ashl3<mode>3_{unsigned,signed} now call to 
gen_ashl3_<supf>_<mode> and that arm_mve_builtins.def use 
ashl3_<supf>_<mode> instead of this, <mode> needs to be at the end of 
the name for the builtin construct. Whether this 'form' would work 
everywhere, I don't know. And I suspect you might find more issues like 
this. If there are more than you are willing to change right now then 
maybe the easier step forward is to try to tackle them one at a time, 
and use a new conditional iterator where you've been able to merge NEON 
and MVE patterns.

As a general strategy I think we should try to clean the mess up, but I 
don't think we should try to clean it all up in one go as that will 
probably lead to it not getting done at all. I'm not the maintainer, so 
I'd be curious to see how Kyrill feels about this, but in my opinion we 
should take patches that don't make it less maintainable, so if you can 
clean it up as much as possible, great! Otherwise if its not making the 
mess bigger and its enabling auto-vec then I personally don't see why it 
shouldn't be accepted.
> Or we can keep different expanders for Neon and MVE? But we have
> already quite a few in vec-common.md.
We can't keep different expanders if they expand the same optab with the 
same modes in the same backend. So we will always have to make NEON and 
MVE work together.