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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:99-108
+  enum ScalableForceKind : unsigned {
+    SK_Undefined = UINT_MAX, ///< Not selected.
+    SK_Disabled = 0,         ///< Disables vectorization with scalable vectors.
+    SK_Preferred = 1, ///< Like SK_Enabled, but favoring scalable vectors when
+                      ///  the cost-model is inconclusive. This is the default
+                      ///  (1) when the hint is enabled through a pragma.
+    SK_Enabled = 2,   ///< Vectorize loops using scalable vectors or fixed-width
----------------
paulwalker-arm wrote:
> paulwalker-arm wrote:
> > Given the way the code is written, for example `isScalableVectorizationPreferred()` and the comments attached to the enum values I think the patch will be simpler if the enum names reflected their intent.  For example:
> > ```
> > Unspecified,
> > FixedOnly,
> > PreferFixed,
> > PreferScalable
> > ```
> It looks weird to have two `Kind`'s that have different definition/handling for `Undefined`.
That makes the code a bit clearer indeed, so I've adopted your suggestion. I have left the external user-facing option the same though.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:108
+                      ///  fixed-width vectors when the cost is inconclusive.
+  };
+
----------------
vkmr wrote:
> The ordering of `SK_Enabled` and `SK_Preferred` feels non-intuitive. Is it because of the check:
> ```
> case HK_SCALABLE:
>     return (Val == SK_Disabled || Val == SK_Preferred);
> ```
> that can only check `Val` to be 0 or 1?
Yes the LLVM IR Metadata attribute is a boolean 0 or 1. I agree this is a bit non-intuitive, so I've fixed this now by passing the argument to `Hint::validate` as a reference, so that we can remap the value.


================
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) {
----------------
vkmr wrote:
> sdesmalen wrote:
> > 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).
> If I understand correctly, metadata hints take precedence over this flag unless the flag is set to `off`?
> If this is the case and the default value of the flag (for now) is set to `SK_Disabled`, that means the metadata hints won't have an effect if the flag is not explicitly set to `on` or `preferred`, right?
Yes, that's right. This allows people to start experimenting with scalable vectors and using the pragma in their code, without it possibly breaking things unless they compile with the flag `-scalable-vectorization=on|preferred`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101945