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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen 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;
----------------
MaxSafeVectorWidthInBits can't exceed UINT_MAX. If this this early bail out isn't strictly necessary, I'd suggest removing it.


================
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;
+      }
----------------
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.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5609
+      return computeFeasibleMaxVF(
+          ConstTripCount, ElementCount::getFixed(UserVF.getKnownMinValue()));
+    }
----------------
Can it know that `ElementCount::getFixed(UserVF.getKnownMinValue())` would be a valid VF? (that check is below on line 5614).
Probably good to have a test for that as well.


================
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
----------------
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.


================
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
----------------
Why does it require asserts?


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

https://reviews.llvm.org/D91718