[PATCH] D91718: [LV] Legalize scalable VF hints

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5574
+    // Nothing to do if there are no dependencies.
+    if (MaxSafeVectorWidthInBits >= UINT_MAX)
       return UserVF;
----------------
sdesmalen wrote:
> MaxSafeVectorWidthInBits can't exceed UINT_MAX. If this this early bail out isn't strictly necessary, I'd suggest removing it.
> MaxSafeVectorWidthInBits can't exceed UINT_MAX. If this this early bail out isn't strictly necessary, I'd suggest removing it.

There's a couple of existing tests:
* Transforms/LoopVectorize/scalable-loop-unpredicated-body-scalar-tail.ll
* Transforms/LoopVectorize/metadata-width.ll
that will fall over if the hint is ignored when scalable vectorization isn't supported. I suppose those could become target-specific tests.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5579-5585
+      if (!TTI.supportsScalableVectors()) {
+        reportVectorizationFailure(
+            "Ignoring scalable VF because target does not support scalable vectors",
+            "Ignoring scalable VF because target does not support scalable vectors.",
+            "NoScalableVectorSupport", ORE, TheLoop);
+        return None;
+      }
----------------
fhahn wrote:
> sdesmalen wrote:
> > Instead of returning 'None' (and thus not allowing any vectorziation), isn't it better to ignore the UserVF  and continuing by allowing a VF to be chosen as normal?
> > 
> > i.e.
> > 
> >   bool IgnoreUserVF = UserVF.isScalable() && TTI.supportsScalableVectors();
> >   if (IgnoreUserVF)
> >     ORE->emit([&]() {
> >        return OptimizationRemark(....) };
> > 
> >   if (UserVF.isNonZero() && !IgnoreUserVF) {
> >     ...
> >   }
> > 
> >   // otherwise, let the LoopVectorizer choose a VF itself.
> > 
> > I guess that can be split out into D93060 as well.
> Agreed, if the user requests scalable vectorization through a hint, I probably should just drop the hint and proceed with normal cost-modeling.
> 
> (if there's a reason to keep the bail-out, can we just return a VF of 1? this is how other places in the function indicate that vectorization should be avoided)
> Instead of returning 'None' (and thus not allowing any vectorziation), isn't it better to ignore the UserVF  and continuing by allowing a VF to be chosen as normal?
>
> ...
> 
> I guess that can be split out into D93060 as well.

Ok np, I'll split that out then.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5604
+      // instead.
+      LLVM_DEBUG(
+          dbgs()
----------------
sdesmalen wrote:
> Should this be an OptRemark instead?
> Should this be an OptRemark instead?

Good point, I'll add that


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7433
 
-  assert(!MaxVF.isScalable() &&
-         "Scalable vectors not yet supported beyond this point");
+  if (MaxVF.isScalable()) {
+    // TODO: Use scalable MaxVF once we've added support for scalable
----------------
sdesmalen wrote:
> We still want to clamp to a smaller scalable VF if `UserVF.isScalable() && MaxVF.isScalable() && UserVF  > MaxVF`. Because the loop below doesn't support scalable VFs, it is best to single that case out.
> 
> That means:
> 1. UserVF = Scalable && MaxVF = Scalable && UserVF > MaxVF => Pick MaxVF
> 2. UserVF = Scalable && MaxVF = Fixed => Fall through into loop below.
> 3. UserVF = Fixed && MaxVF = Fixed && UserVF > MaxVF => Fall through into loop below.
> 
> Where the clamping in 1. is temporary until the loop below works on scalable vectors, so that it can choose the most profitable VF based on the cost-model.
Ok, I'll add a temporary workaround for scalable VFs to select MaxVF above if UserVF is unsafe.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-hint.ll:1
+; REQUIRES: asserts
+; RUN: opt -mtriple=aarch64-none-linux-gnu -mattr=+sve -loop-vectorize -S < %s 2>&1 | FileCheck %s
----------------
sdesmalen wrote:
> Why does it require asserts?
> Why does it require asserts?

Because of the debug flags


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

https://reviews.llvm.org/D91718