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.
