This is an archive of the discontinued LLVM Phabricator instance.

[LV] Enable scalable vectorization by default for SVE cores.
ClosedPublic

Authored by sdesmalen on Dec 13 2021, 10:28 AM.

Details

Summary

The availability of SVE should be sufficient to enable scalable
auto-vectorization.

This patch adds a new TTI interface to query the target what style of
vectorization it wants when scalable vectors are available. For other
targets than AArch64, this currently defaults to 'FixedWidthOnly'.

Diff Detail

Event Timeline

sdesmalen created this revision.Dec 13 2021, 10:28 AM
sdesmalen requested review of this revision.Dec 13 2021, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 10:28 AM
david-arm added inline comments.Dec 16 2021, 3:36 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
171

Given we've moved this out of LoopVectorize.cpp I wonder if it's better not to mention the hints?

1396

Given the class name is ScalableVectorizationKind perhaps it's more consistent to name the function getScalableVectorizationKind() instead, i.e. like in other places where we have things like get'ClassName' or get'Enum'.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
114

Hi @sdesmalen, I personally find it a little wordy to have a variable called PreferredScalableVectorizationKind that itself has enum values with the word Preferred in it, i.e. PreferScalable, etc. Also, one of the kinds is Unspecified, which is not really a statement of preference. What do you think?

sdesmalen updated this revision to Diff 394848.Dec 16 2021, 6:07 AM
sdesmalen marked 3 inline comments as done.

Dropped 'preferred' from the names.

dmgreen added inline comments.Dec 17 2021, 12:31 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
172

There is an extra space in here.

paulwalker-arm added inline comments.Dec 20 2021, 4:53 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
1396

Now this interface has moving outside of LoopVectorize I'm not a fan of the name. It looks weird for getScalableVectorizationKind to return FixedWidthOnly.

From a pure naming point go view perhaps just getVectorizationKind? But then the "kind" is really an expression of the order of things rather than being a specific thing.

Another alternative is to break the question up. We already have TTI.supportsScalableVectors() so what about adding TTI.useScalableVectorization() and TTI.preferScalableVectorization()? Here I think the meaning is more instantly recognisable and it means you don't need to move the enum. What do you think?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
95

What do you think about passing in TTI* instead?

I see the main advantages being we won't need extra parameters for other TTI based questions and for cases like reportVectorizationFailure we don't need to make up some value that may hurt us in the future and can instead just pass in nullptr, which I think makes it clearer that this is not a complete hint.

161

Shouldn't the command line option alway win? What will happen for ForceScalableVectorization==on when the TTI returns FixedWidthOnly?

sdesmalen updated this revision to Diff 395432.Dec 20 2021, 6:45 AM
sdesmalen marked an inline comment as done.
  • Added TargetTransformInfo::enableScalableVectorization() in favour of ScalableVectorizationKind.
  • Removed "scalable-vectorization=preferred", so that "scalable-vectorization=on" always implies 'preferred'.
  • LLVM force options now always override.
sdesmalen marked 3 inline comments as done.Dec 20 2021, 6:47 AM
sdesmalen added inline comments.
llvm/include/llvm/Analysis/TargetTransformInfo.h
1396

I implemented this suggestion, but also removed prefer, so that on always implies prefers scalable vectors. That simplified the implementation quite a bit!

paulwalker-arm accepted this revision.Dec 20 2021, 7:37 AM

A few things to consider plus a bug in ForceScalableVectorization's help text but otherwise this looks good to me.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
108

rogue comma

125

I guess by this point you're saying Scalable is now effectively a bool so perhaps just (Width.Value, Scalable.Value)?

Either way, we shouldn't use magic values so either treat it as a bool or use the enum values.

147

As above perhaps !Scalable.Value?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
69–70

You've kept the wrong help text.

135

"Scalable vectorization is disabled if no preference is specified"?

This revision is now accepted and ready to land.Dec 20 2021, 7:37 AM
sdesmalen updated this revision to Diff 395451.Dec 20 2021, 7:47 AM
sdesmalen marked 6 inline comments as done.

Addressed comments.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
69–70

Thanks, good spot!

This revision was landed with ongoing or failed builds.Dec 20 2021, 8:23 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 20 2021, 9:09 AM

Looks like this broke check-llvm: https://lab.llvm.org/buildbot/#/builders/109/builds/28397

Please take a look, and revert for now if it takes a while to fix.

Looks like this broke check-llvm: https://lab.llvm.org/buildbot/#/builders/109/builds/28397

Please take a look, and revert for now if it takes a while to fix.

Thanks for the report. I'm investigating now but it looks like it'll be an easy fix.

Thanks, I've fixed it in rG290ae657a61d.