This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Disable (SLP|Loop)Vectorizer when function may be executed in streaming mode.
ClosedPublic

Authored by sdesmalen on Oct 14 2022, 1:12 AM.

Details

Summary

When the SME attributes tell that a function is or may be executed in Streaming
SVE mode, we currently need to be conservative and disable _any_ vectorization
(fixed or scalable) because the code-generator does not yet support generating
streaming-compatible code.

Scalable auto-vec will be gradually enabled in the future when we have
confidence that the loop-vectorizer won't use any SVE or NEON instructions
that are illegal in Streaming SVE mode.

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 14 2022, 1:12 AM
sdesmalen requested review of this revision.Oct 14 2022, 1:12 AM
sdesmalen updated this revision to Diff 467711.Oct 14 2022, 1:26 AM

Simplified test using 'sed' macro replacement.

paulwalker-arm accepted this revision.Oct 16 2022, 8:55 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64Subtarget.h
124

Likely doesn't matter much but the specification uses StreamingSVEMode.

This revision is now accepted and ready to land.Oct 16 2022, 8:55 AM

I agree that the idea here sounds sensible. I feel it might be better to have the variable store isStreamingModeEnabled though. That seems like the standard default, and helps avoids a lot of double negatives.

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
428

This seems to be in the wrong place, between "SVEMax" and MaxSVEVectorSize.

llvm/test/Transforms/LoopVectorize/AArch64/sme-vectorize.ll
84

Are you sure this tests anything in SLP?

If you do go for a rename can I recommend InStreamingSVEMode :)

The reason to go for IsStreamingModeDisabled (rather than Enabled) is to also cover the case of a 'streaming compatible' function, where streaming mode may be enabled, but we don't know at compile-time whether it is.

Sorry @sdesmalen, I must keep reminding myself that streaming compatible is a subset of streaming SVE mode :) What about representing each separately? For example IsStreamingSVECompatible and InStreamingSVEMode. The latter likely has no current uses? but will allow that difference between "streaming compatible" and "streaming SVE mode" to be queried if/when necessary.

Sorry @sdesmalen, I must keep reminding myself that streaming compatible is a subset of streaming SVE mode :) What about representing each separately? For example IsStreamingSVECompatible and InStreamingSVEMode. The latter likely has no current uses? but will allow that difference between "streaming compatible" and "streaming SVE mode" to be queried if/when necessary.

In that case we'd have the possibility of representing an invalid combination (i.e. a function cannot be both StreamingCompatible and StreamingSVEMode), so then an enum would be a better choice. But since there is (currently) no need for this, is IsStreamingSVEDisabled not better at this point? It makes little sense to add something we don't need and can easily extend later.

llvm/test/Transforms/LoopVectorize/AArch64/sme-vectorize.ll
84

I believe so, the hints passed to LoopVectorize sets the vectorize-width to 1 and the interleave-count to 4 using the attributes. We end up with a fixed-width vector code, so this must be due to SLP.

paulwalker-arm added a comment.EditedOct 18 2022, 10:31 AM

Sorry @sdesmalen, I must keep reminding myself that streaming compatible is a subset of streaming SVE mode :) What about representing each separately? For example IsStreamingSVECompatible and InStreamingSVEMode. The latter likely has no current uses? but will allow that difference between "streaming compatible" and "streaming SVE mode" to be queried if/when necessary.

In that case we'd have the possibility of representing an invalid combination (i.e. a function cannot be both StreamingCompatible and StreamingSVEMode), so then an enum would be a better choice. But since there is (currently) no need for this, is IsStreamingSVEDisabled not better at this point? It makes little sense to add something we don't need and can easily extend later.

I was considering StreamingCompatible as being a subset of StreamingSVEMode so both could be true. That said, I do agree, the exact mechanics can be changed at a later point once we understand the interactions more. This is why I previously/already accepted the patch because I'm happy with the current intent.

Matt added a subscriber: Matt.Oct 19 2022, 5:20 AM
This revision was landed with ongoing or failed builds.Oct 19 2022, 9:43 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen added inline comments.Oct 19 2022, 10:03 AM
llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
428

Sorry this comment escaped my attention, I've pushed a fix separately in rG36864d47d6b0