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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 4 inline comments as done.
sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:147
+  /// the cost-model is inconclusive.
+  bool isScalableVectorizationPreferred() const {
+    return Scalable.Value == SK_Preferred;
----------------
CarolineConcatto wrote:
> This function is not used.
Okay, I can move this to the other patch.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:86
+  case HK_SCALABLE:
+    return (Val == SK_Disabled || Val == SK_Preferred);
   }
----------------
CarolineConcatto wrote:
> Should it be all the possible solutions here:
> Val == SK_Disabled
> Val == SK_Preferred
> Val == SK_Enabled
> Val == SK_Undefined
> 
Perhaps at some point, but at the moment the value is interpeted as a boolean, and Clang sets it only to '0' or '1', so if we want to expand it to support other values in the metadata, we'll need to update Clang as well (which is something or another patch).


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:99
       Predicate("vectorize.predicate.enable", FK_Undefined, HK_PREDICATE),
-      Scalable("vectorize.scalable.enable", false, HK_SCALABLE), TheLoop(L),
-      ORE(ORE) {
+      Scalable("vectorize.scalable.enable", SK_Undefined, HK_SCALABLE),
+      TheLoop(L), ORE(ORE) {
----------------
CarolineConcatto wrote:
> Can vectorize.scalable.enable have the same values as scalable-vectorization?
> Which values should  LoopVectorizeHints::Hint::validate check?
So first the variable `Scalable` is initialized with SK_Undefined.
* Then it looks through the metadata to see if the front-end has set the value (which can only be 'disabled' or 'preferred', which is checked by LoopVectorizeHints::Hint::validate)
* If it is not set by the metadata, then it will see if the flag is set to force scalable vectorization one way or another.
* Otherwise, if it is set by the metadata, it can be overriden by the flag only if the value is 'disable'.

This means it is possible to disable scalable vectorization using a single flag. But otherwise, metadata (because it is more specific) takes precedence over the flag (which is more generic).


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:117
+    Scalable.Value = SK_Disabled;
+
   if (IsVectorized.Value != 1)
----------------
CarolineConcatto wrote:
> Just to double check:
>  if **Scalable.Value != SK_Undefined**  and ** ScalableVectorization != SK_Disabled** 
>  **Scalable.value** will have the value set in **vectorize.scalable.enable**, is that correct?
>  and not in 
> **scalable-vectorization**
Yes, that's correct. In that case it will have the value from the metadata.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-analysis.ll:14
+; CHECK_SCALABLE_PREFERRED: LV: Found feasible scalable VF = vscale x 4
+; CHECK_SCALABLE_DISABLED-NOT: LV: Found feasible scalable VF
 ; CHECK_SCALABLE_ON_MAXBW: LV: Found feasible scalable VF = vscale x 16
----------------
CarolineConcatto wrote:
> Is there a way to tell that VF scalable will not be used?
> Only this:
> **LV: Found feasible scalable VF**
> is quite odd, because it does not explicitly says it will not use scalable.
It has the `-NOT` as part of the check prefix though.

e.g. CHECK-NOT: foo
will cause FileCheck to fail if `foo` occurs.

In this case, the prefix is `CHECK_SCALABLE_DISABLED` instead of `CHECK`, so `CHECK_SCALABLE_DISABLED-NOT` fails if `LV: Found feasible scalable VF` occurs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101945