[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.

In D90247#2357093, @nikic wrote:

Ok, I can build that out. Are we okay with the suboptimal legalization though? I'll wait for that decision before putting more time into this.

Or does anyone see a clever fix for the illegal type legalization? It looks like we lost information during widening, so I'm not sure we can get it back in a non-hacky way.

Not sure I follow. If the neutral element is fixed, then the extra fadds should also get folded away. Or is there some additional sub-optimality here?

In D90247#2357013, @nikic wrote:

A good example of this can be seen in @test_v3f32 from vecreduce-fadd-legalization-strict.ll. Here we end up with 4 FADDs, instead of the 3 FADDs required. The newly added FADD is the result of widening the illegal v3f32 vector type to v4f32, where the newly added element in the reduction is the "neutral" value, 0.0.

Looking at https://github.com/llvm/llvm-project/blob/5a3ef55a524bf9e072d98286e5febdb218b1fc72/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L7477-L7480, shouldn't this just be a matter of using -0.0 as the neutral element instead? If 0.0 is not actually neutral here, then this is not just suboptimal, it's incorrect. (We should fix this for the non-sequential case as well.)

[1] I just wanted to highlight my previous VBITS_EQ_256-COUNT-33: fadd comment as this gives us a bit more test coverage and is something that will obviously fail (in a good way) when the splitting work is available.

@paulwalker-arm, back to the splitting discussion...

In D89162#2350619, @nikic wrote:

The eventual goal here is to expand reductions during legalization only. The IR expansion exists because the DAG legalization support has been patchy historically, with VECREDUCE_SEQ_* being the last remaining hole.

Updating patch, but not ready for a serious review yet as I haven't started the splitting work. I'm still not convinced we can handle splitting appropriately with the current setup, but will comment on that seperately.

In D89162#2350199, @paulwalker-arm wrote:

In D89162#2347814, @cameron.mcinally wrote:

The new tests would be broken without the legalisation changes, so I'm assuming that those are enough coverage. Maybe I'm missing something though...

Are you sure? I took your patch for a test drive and removed all but the TLI.getOperationAction related change from Legalize*.{cpp, h} and the tests passed.

In D89162#2347701, @paulwalker-arm wrote:

Sorry @cameron.mcinally I've not had much time for code reviews this week although will take proper look tomorrow. I have a question though. You've added extra legalisation support but I don't see any explicit tests (or at least ones with matching check lines) for it. Is this something you need for this patch? (I'm guessing sve-fixed-length-fp-reduce.ll's stock NEON run line triggers the cases?) If so then there really should be a neon specific test file that verifies the widening and scalarisation changes as the NEON run line for the "fixed-length" tests is more about ensuring no SVE instructions slip through.

Try again with 80 column fix...

Fix 80 column issue. No other changes intended...

In D88707#2335496, @aemerson wrote:

Can you update the tests to use the new non-experimental intrinsic name?

Some other notes:

It looks like NEON FADDA support is missing upstream too.

This is ready for review now...

Not lowering to SVE for v2f## MVTs makes sense for now but as before when we have proper support for v#i1 our hands will be tied.

Oh, and I can do VECREDUCE_FADD first. Just hit VECREDUCE_SEQ_FADD before I realized I need to add 'fast' to the intrinsic calls.

You're right on all the comments. I stopped midway when I hit the legalisation issue, so that's why this patch is rough. That would need to be built out first before this work could continue. And I think NEON support should be built out before that. It sounds like I'm not stepping on toes, so I'll go in that order. Thanks.

Legalisation tests added for sve-fixed-length-fp-reduce.ll in:

Correct OR->EOR variable name.

Legalisation tests added for sve-fixed-length-int-reduce.ll in:

LGTM

Committed. It looks like the legalisations seem reasonable. Something like:

In D88707#2308382, @paulwalker-arm wrote:

we start adding proper v#i1 support

LGTM

Ok, I think that's all of them. Looks like it started with D87796 and was buried in other changes. To confirm: