[PATCH] D101945: [LV] Add -scalable-vectorization=<option> flag.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

A few stylistic things to consider that I'll not hold you to, but the change to `validate` is the main thing stopping me from accepting the patch.



================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:65-68
+    /// \return true if \p Val is a valid value for the Hint. \p Hint is passed
+    /// by reference, so that the value can be transformed. For example, to map
+    /// the value from a boolean (i1) to a specific enum value.
+    bool validate(unsigned &Val);
----------------
See comment on implementation.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:102-114
+  enum ScalableForceKind {
+    SK_Unspecified = -1,   ///< Not selected.
+    SK_FixedWidthOnly = 0, ///< Disables vectorization with scalable vectors.
+    SK_PreferFixedWidth =
+        1, ///< Vectorize loops using scalable vectors or fixed-width
+           ///  vectors, but favor fixed-width vectors when the cost is
+           ///  inconclusive.
----------------
Personally I think the following is prettier and more in keeping of the coding style used by other well documented enums:
```
enum ScalableForceKind {
  /// Not selected.
  SK_Unspecified = -1,   
  /// Disables vectorization with scalable vectors.
  SK_FixedWidthOnly = 0,
  /// Vectorize loops using scalable vectors or fixed-width
  /// vectors, but favor fixed-width vectors when the cost is
  /// inconclusive.
  SK_PreferFixedWidth = 1, 
  /// Vectorize loops using scalable vectors or fixed-width
  /// vectors, but favor scalable vectors when the cost-model is
  /// inconclusive. This is the default when the scalable.enable
  ///  hint is enabled through a pragma.
  SK_PreferScalable = 2
```


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:152-153
+  bool isScalableVectorizationExplicitlyEnabled() const {
+    return (ScalableForceKind)Scalable.Value != SK_FixedWidthOnly &&
+           (ScalableForceKind)Scalable.Value != SK_Unspecified;
+  }
----------------
Perhaps nit picking but given the function says ExplicitlyEnabled, would it make more sense to explicitly compare against `SK_PreferFixedWidth` and `SK_PreferScalable`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:60
+        clEnumValN(LoopVectorizeHints::SK_FixedWidthOnly, "off",
+                   "Disables vectorization with scalable vectors."),
+        clEnumValN(LoopVectorizeHints::SK_PreferFixedWidth, "on",
----------------
What about `Scalable vectorisation is disabled.`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:62-64
+                   "Vectorize loops using scalable vectors or fixed-width "
+                   "vectors, but favor fixed-width vectors when the cost is "
+                   "inconclusive."),
----------------
What about `Scalable vectorisation available, but favor fixed-width vectorisation when the cost is inconclusive.`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:66-67
+        clEnumValN(LoopVectorizeHints::SK_PreferScalable, "preferred",
+                   "Like 'on', but favoring scalable vectors when the "
+                   "cost-model is inconclusive.")));
+
----------------
What about `Scalable vectorisation available and favored when the cost is inconclusive.`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:74
 
-bool LoopVectorizeHints::Hint::validate(unsigned Val) {
+bool LoopVectorizeHints::Hint::validate(unsigned &Val) {
   switch (Kind) {
----------------
I realise you've changed this in response to a previous comment but having a validation function change the value it's validating seems wrong.  You could change the function name or perhaps return an optional or something but given the enum values have been renamed I wonder if the original comment has been nullified?

That's to say, whilst I agreed with the comment's original context, I don't see any loss of intuition if the renamed enum order became:
```
SK_FixedWidthOnly = 0
SK_PreferScalable = 1
SK_PreferFixedWidth = 2
```
and thus you would no longer need these changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101945