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:

Sorry again, Paul. Still looking at how far this propagated. Looks like it was introduced with the VECREDUC_ADD patch. Need some more time...

Update more typos...

Address reviews...

Also, while I have everyone's attention, there are a number of unhandled vector reduction intrinsics with SVE support. Do we want to add lowerings for those? E.g. ANDV.

In D88317#2297767, @paulwalker-arm wrote:

Just a heads up that @kmclaughlin is starting to look at legalisation/lowering for scalable vector types. From an operation legalisation point of view you've done most of the plumbing so hopefully any toe treading will be minimal.

Correct bad string replacement.

Cray/HPE is motivated to see this patch land. My confidence is low in this area, but I don't see any review comments that are fatal. Any reason we can't move forward with this change?

How about something like this? Not really exhaustive coverage, but it matches the current legalization tests for the reduce intrinsics.

You're right about v2i64 UADDV. The existing test didn't catch my regression, so I added a new CHECK line to that test. Also added a line to Custom lower that as before.

LGTM

LGTM with moderate confidence. Does anyone see a reason that this shouldn't land?

LGTM with moderate confidence. Does anyone see a reason that this shouldn't land?

A few other cleanliness opportunities popped up during the refactor. How about something like this?

Thanks for the patches, @paulwalker-arm. Those helped a lot.

This is certainly not the expected behaviour so I'll get it fixed.

Here's the most obvious choice -- glueing the new check to the AArch64ISD::EXT generation. That's good because the intention is obvious, and future code changes won't get in the way. But it could also be argued that the NEON-sized vector check should precede this entire if-else statement. I.e. the EXTRACT_SUBVECTORs have the same problem as AArch64ISD::EXT. None of this code is ready for SVE-sized fixed vectors.

Remove the BUILD_VECTOR->EXTRACT_SUBVECTOR transform so that this is easier to review. Will post a separate Diff for that.

Fix comment.

Turns out that ReconstructShuffle(...) needs a lot of work to be updated. The EXTRACT_SUBVECTOR index assumes we're extracting a 1/2 width vector. I missed that.

Update tests -- don't use pointer args for NEON sized vectors.

Fix bad copy-and-paste in CHECK lines. No significant changes made.

Updating Diff for recent upstream changes.

In D86894#2250381, @efriedma wrote:

Is there any way to write a test for the transform? Maybe not, given the limited places we use INSERT_SUBVECTOR.

Updated with @david-arm's comments for Thursday's Sync-up call.

Thanks, David. Ok, I'll put a pin in this until Thursday's call. But yeah, isKnownMultipleOf(...) sounds fair. I'll prepare a patch to see if any surprises come up.

I don't really have an opinion on any of this. Just thinking aloud...

Ok, I guess that makes sense. So if we're considering Min elements, should we just overload % then?

˜Bah, egg on my face. You're right that D86394 fixes the immediate issue. Sorry for the noise.

D86394 addresses a different issue, I think. Posted some new code in D85364 to expose the AArch64ISD::ZERO_EXTEND_INREG_MERGE_PASSTHRU issue.