Updated patch for Matt's reviews...

Update the diff to include context.

Created D144571 with a patch to fix the ICE.

Also note that a fix will be needed for the release/16.x branch.

Hi Matt,

LGTM

Apologies, @efriedma. I've updated the patch again to move the check into collectSRATypes(). Would you mind doing one more review?

Not sure why you're sticking the test in llvm/test/CodeGen/AArch64 instead of llvm/test/Transforms/GlobalOpt/.

Ah, great. Thanks for working on this.

Just spoke with Paul about this issue. We decided that defining the undef elements may be too aggressive. Updated patch to come...

Addressed Paul's review...

Updated Diff to implement commuting with PatFrags.

LGTM. Thanks, Paul.

Good point. Replacing the lowered truncates with ptrue sounds like a win in the general case. Abandoning this Diff.

Updated patch based on @david-arm's review.

Fix formatting for the Lint bots.

In D120152#3332664, @tschuett wrote:

Why didn't or cannot InstCombine catch this?

Updated Diff.

Ah, sorry for the noise. Abandoning this Diff...

In D119252#3307242, @david-arm wrote:

Hi @paulwalker-arm, this bug was actually found in user-written code in Gromacs, although only there was only one instance of this I think. So it is something users may see, just not very often!

I believe this is a duplicate of D117674, which had not been reviewed yet so I've just pushed it along.

Further reduced test case, but still not great.

Is it possible to break the 4 subtasks into separate reviews?

Perhaps worth adding matching half/fp16 tests to sve-fixed-length-fp-fma.ll but otherwise looks good.

Updated Diff for @paulwalker-arm's reviews...

Fix clang-format warning and add the missing '+' to "+sve".

[NOT READY FOR REVIEW]

In D94444#2497697, @paulwalker-arm wrote:
<A x Elt> llvm.experimental.vector.extract.elements(<B x Elt> %invec, i32 index, i32 stride)

Ok, I see where you are coming from now. LoopVectorize is keeping the shuffle result full by widening the the load+shuffle to double wide. LV's double wide choice seems like a weird one, but I suppose if that sequence is codegen'd correctly, then it will work out.

In D94444#2497697, @paulwalker-arm wrote:
<A x Elt> llvm.experimental.vector.extract.elements(<B x Elt> %invec, i32 index, i32 stride)

Having said that, I wonder if we should revisit the idea of allowing shuffle vectors to accept step vector masks?

In D94444, @paulwalker-arm proposed a more generic extract vector intrinsic that accepts an index and stride. Now I'm wondering if we should just have a generic scalable shuffle vector intrinsic to handle all these operations under one intrinsic.

In D94444#2497697, @paulwalker-arm wrote:

A bit of a flyby review as I'm still on holidays but to my mind many of the restrictions being proposed for the new intrinsic seem purely down to the design decision of splitting the input vector across two operands. I understand this is how the underlying instructions work for SVE but that does not seem like a good enough reason to compromise the IR.

So my first questions are whether the IR and ISD interfaces need to match and from an IR point of view what is the expected usage?

Add known minimum number of elements restrictions...

Updated to @david-arm's suggested naming scheme...

I'm assuming scheduling the new addvls closer to their uses is a register pressure win?

Address some of @sdesmalen's comments, but deferring name changes...

In D94444#2492146, @sdesmalen wrote:

Thanks for creating this patch!

I chose to extract the even elements from a pair of vectors (full vector result), rather than a single vector (1/2 width vector result). This is in line with existing fixed shuffle vectors. And can be extended to accept an undef argument if needed. The motivation behind this decision was that we'd want the result vector to be a full vector for performance reasons. It would also map well to SVE's LD2 and UZP1.

Are you also planning to add intrinsics for interleaving?

In D94193#2483858, @paulwalker-arm wrote:

Please can you add entries for nxv2f16 as well? That way all the legal fp types are covered.

LGTM

Ping.

Add FIXME comment.

LGTM

LGTM with one nit below...

I think @ctetreau's "first class citizen" argument on the RFC has merit though. But this patch is a good first step if we're not ready to extend ShuffleVector yet. I personally would like to see ShuffleVector extended eventually, since it would be easier to optimize.

Do we need to protect against mismatched element types? Or does legalization handle those exts/truncs?

In D90644#2372260, @cameron.mcinally wrote:

In D90644#2372150, @nikic wrote:

• In llvm/test/CodeGen/ARM/vecreduce-fmul-legalization-strict.ll and llvm/test/CodeGen/AArch64/vecreduce-fmul-legalization-strict.ll, use 1.0 instead of 0.0 as the start value. That was probably a copy&paste mistake from fadds.

That caught my eye too, but the 0.0 seemed okay since we can't peep this without NSZ (-0*0) and NNAN (0*NaN). Changing it to 1.0 isn't a big deal though...

In D90644#2372150, @nikic wrote:

• In llvm/test/CodeGen/ARM/vecreduce-fmul-legalization-strict.ll and llvm/test/CodeGen/AArch64/vecreduce-fmul-legalization-strict.ll, use 1.0 instead of 0.0 as the start value. That was probably a copy&paste mistake from fadds.

Reformat to appease pre-merge checks...

Update patch based on @nikic's comments...

Updated patch with, I think, all the needed legalizations.

Comment from ARM/ARMISelLowering.cpp:

Ah, I see it in ARM/. That will work...

Update 'neutral' element to -0.0.