Nice cleanup.

This change looks right to me.

In D125699#3517128, @arsenm wrote:

I feel like I fixed this one before

We have a similar fix downstream and I actually thought we already upstreamed this. In any case, good fix!

In D125449#3508487, @frasercrmck wrote:

From what I gather about the bug report, RISCV doesn't suffer from it as we always dyn_cast to Instruction. Is that right? Just wondering if we should add a similar test - I don't think we have constant expr coverage in sink-splat-operands.ll.

That's what I did initially, but I couldn't write a test with a ZExt/SExt as ConstExpr that wouldn't fold, so that would mean I'd rewrite the code without being able to test it.

• Addressed comments.
• Reverted the fixed-width change (leading to different addrspacecast constant)

The tests all seem valid, but it's a bit difficult to review this patch without knowing what it's really trying to test without really going through AArch64TargetLowering::ReconstructShuffle in detail. That function is quite big/complicated, so it's not trivial to spot whether anything is missing or redundant.
My main question is whether all the insert/extract-subvector tests need to test all the permutations of element type and subvector?

Thanks for working on this @RosieSumpter!

Looks like a nice improvement and the code is now a bit more sensible.

Did you do any performance measurements to get an impression of the performance impact of this change?

Thanks for this patch Dave, I've left a few comments.

Thanks for this @RosieSumpter. If you remove the 'WIP' and s/SVE/SVE2/ from the title and description, I'm happy to accept. The negative tests for the SVE(1) intrinsics can be added as a separate patch.

Hi @t.p.northover, I've added you as a reviewer since you're the AArch64 backend maintainer and the approach set out in this patch has some implications on (forward-)compatibility of the IR.

Updated some of the comments and changed assembler behaviour to let +sme
enable all streaming-compatible SVE(2) instructions without necessarily
requiring +sve2 as well.

Hi @rsandifo-arm, thanks for your comments!

Thanks for the fix @malharJ, I'm happy with the patch now.

Simplified the patch by setting +pstate-sm1 and +pstate-za1 by default,
as neither of these have any effect without +sme. This reduces the need
to have separate internal/external flags for SME like in the first
revision because having +sme imply +pstate-sm1 and +pstate-za1 led
to problems when disabling the pstate features (which is kind of their
intended use).

Hi @TKaipeng, this looks like a duplicate of D121452.

Hi @malharJ, thanks for this fix! I believe this is doing the right thing, because for scalable vectors an instruction should either be considered uniform after vectorization, or it shouldn't be considered scalar.

D121297 has been committed, so I think this patch should be good to land.

In D121294#3370132, @MattDevereau wrote:

In D121294#3370085, @sdesmalen wrote:

This is missing tests for svundef, svrdffr, svsetffr and svpfalse?

@sdesmalen Only svcnt and svptrue cause the crash. This might be because of extra values added by the [IsAppendSVALL] TypeFlag in arm_sve.td. We decided to change other intrinsics with 0 parameters as there seems to be no drawbacks to doing so and it might catch further future errors.

Fair enough. Since you're adding the void to the prototype for a reason (the diagnostic behaviour now changes), I figured you may as well want to test it.

This is missing tests for svundef, svrdffr, svsetffr and svpfalse?

Showing a failure on build failures is one of the things that we rely on the buildbots for, so I agree it makes sense to check for NOEXE. Thanks for the fix!

I've seen this in other places as well where introducing explicit implicit defs leads to less efficient register allocation, so I can see this is an improvement.

Apologies for the delay in reviewing, but this looks good to me.

Thanks for the update, LGTM!

Looks accurate, thanks!

• Replaced nonstreaming-compatible-sme with streaming-agnostic-sme
• Replaced streaming-sme with streaming-sve.

Added condition to findMoreOptimalIndexType that index is a vector of
i64's and swapped order of shl/mul since scale is guaranteed to be constant.

Addressed @david-arm's comments.

Addressed @c-rhodes' comments.

LGTM!

Thanks, LGTM!

In D119529#3321740, @Jim wrote:

In D119529#3313680, @sdesmalen wrote:

Can you add a test for this change?

The crash only triggered target is given (specify -mtriple=riscv64) and BasicTTIImplBase::getCommonMaskedMemoryOpCost would be called.
If no target given, NoTTIImpl is used as TTI, and TargetTransformInfoImpl.h::getMemoryOpCost simply returns cost 1 without any crash.

I don't have plan to implement hook getMaskedMemoryOpCost for RISCV.
Do you have any suggest for this kind situation?

Hi @kmclaughlin, thanks for updating the tests as MIR tests, these look really good now! I've still requested changes on the patch, because the scaling doesn't look right yet. It seems like this offsets the base pointer with the wrong value/offset. It should be pretty trivial to fix that though.