[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
 
+  enum ScalableForceKind {
+    /// Not selected.
----------------
fhahn wrote:
> paulwalker-arm wrote:
> > fhahn wrote:
> > > paulwalker-arm wrote:
> > > > sdesmalen wrote:
> > > > > fhahn wrote:
> > > > > > can this be an enum class or is the code converting from ints to enum values somewhere?
> > > > > There are several conversions between unsigned and the enum, so this requires adding casts to several places.
> > > > I prefer that we remain consistent with the other `Kind` enums.
> > > Using an `enum class` enforces stricter guarantees and allows the compiler the emit better warnings, which seem like tangible practical benefits. I'm not sure if consistency with other enums outweighs the practical benefits? It's quite common for parts of the codebase to evolve step-by-step.
> > > 
> > > Using an `enum` until the other enums are migrated leaves the door open for changes that make switching to an `enum class` harder.
> > Sure I get that. I'm just saying I prefer all the enums associated with the LoopVectorizeHints class to be handled the same way.
> > There are several conversions between unsigned and the enum, so this requires adding casts to several places.
> 
> I missed that comment earlier. If there's the need for casting, it's a moot point anyways.
Np, thanks for the suggestion anyway!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:60
+        clEnumValN(LoopVectorizeHints::SK_FixedWidthOnly, "off",
+                   "Scalable vectorisation is disabled."),
+        clEnumValN(LoopVectorizeHints::SK_PreferFixedWidth, "on",
----------------
vkmr wrote:
> paulwalker-arm wrote:
> > Sorry my bad :) you probably want `s/vectorisation/vectorization/`
> Perhaps I am bike-shedding a bit much here (and I am not too sure about this), but the "on" and "off" terminology indicates binary choice, not making it readily clear that there is a possible third choice "preferred". Also reading `-scalable-vectorization=on` doesn't make it clear that it in fact prefers fixed vectorization.
> May be something like "off", "possible" and "preferred" is clearer. 
> Thoughts?
It's a good question! From my perspective, having the flag set to 'on' kind of does what it promises, it enables scalable vectorization. It doesn't really suggest to what extend it enables scalable vectorization, which leaves us some room to play with what a good default is. Whereas 'preferred' suggests that scalable vectorization is the preferred way to vectorize loops, instead of fixed-width. I guess the interpretation of 'on' isn't too dissimilar to 'possible', but while the latter is more accurate for now, the former may be more accurate going forward. Unless you feel strongly about this, I have a slight preference to keep 'on' as this feels a bit more future-proof as a default.

I'm not too worried about having much confusion about this, because `--help-hidden` shows all three options with their meaning, and it's not really meant to be a user-facing option in the sense that end-users will see it, but rather an experimental option for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101945