[PATCH] D115651: [LV] Enable scalable vectorization by default for SVE cores.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1409
+  /// \return the preferred style of vectorization.
+  ScalableVectorizationKind getScalableVectorizationKind() const;
+
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:98
                                        bool InterleaveOnlyWhenForced,
+                                       ScalableVectorizationKind Scalable,
                                        OptimizationRemarkEmitter &ORE)
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:140
+  // loop hints.
+  if (ForceScalableVectorization == ScalableVectorizationKind::FixedWidthOnly)
+    ScalableVectorization.Value =
----------------
Shouldn't the command line option alway win? What will happen for `ForceScalableVectorization==on` when the TTI returns `FixedWidthOnly`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115651/new/

https://reviews.llvm.org/D115651