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

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Hey @sdesmalen 
Thank you for the patch.  Nice to see the famous flag in Phabricator.
I have some comments. 
I hope they are not too non-sense.



================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:132
 
-  bool isScalable() const { return Scalable.Value; }
 
----------------
I don't understand where is this function is used, because you've replaced it by isScalableVectorizationPreferred, but you did not changed in any other place. Maybe I am missing something.
And none of them are used.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:146
+  bool isScalableVectorizationPreferred() const {
+    return Scalable.Value == SK_Preferred;
+  }
----------------
What will happened when  Scalable.Value  is SK_Enabled?
It will not be scalable?


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:150
+  /// \return true if scalable vectorization has been explicitly disabled.
+  bool disableScalableVectorization() const {
+    return Scalable.Value == SK_Disabled;
----------------
nit: isScalableVectorizationDisable()


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:109
+  // loop hint.
+  if (ScalableVectorization == SK_Disabled)
+    Scalable.Value = SK_Disabled;
----------------
Do we needs this?
Does this line do the same:
Scalable("vectorize.scalable.enable", ScalableVectorization, HK_SCALABLE),?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101945