[PATCH] D98509: [LV] Calculate max feasible scalable VF.

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:138
+    bool IsForcedScalable = (Scalable.Value == LoopVectorizeHints::FK_Enabled);
+    return Width.Value == 0 && IsForcedScalable;
+  }
----------------
Hi @sdesmalen, to be honest I'm not sure I fully understand this. If the user has set the pragma:

  #pragma clang loop vectorize_width(4, scalable)

then haven't they also explicitly disabled fixed width? Maybe it's just the name of the function that confuses me a little, since from the user's perspective it feels like

  #pragma clang loop vectorize_width(4, scalable)

and

  #pragma clang loop vectorize_width(scalable)

are both disabling fixed width. I thought they were both hints that can be dropped by the compiler if necessary?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1624
 
+  /// \return the maximum vector size based on the target's vector registers,
+  /// limited to MaxVF. This is a helper function of computeFeasibleMaxVF.
----------------
Should this be `element count`, since that's what the function returns here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5631
+
+    assert(ElementCount::isKnownGT(UserVF, MaxSafeUserVF));
+
----------------
Isn't this always going to fire for cases where UserVF is scalable and we cannot vectorise reductions? For example, getMaxLegalScalableVF returns ElementCount::getScalable(0) in this case.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5674
+                                  MaxSafeScalableVF))
+    if (Max.isScalable())
+      LLVM_DEBUG(dbgs() << "LV: Found feasible scalable VF = " << Max << "\n");
----------------
Are you deliberately ignoring the scalable case for now because this will be addressed in a later patch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5847
 
-  assert(MaxVectorSize.getFixedValue() <= WidestRegister &&
-         "Did not expect to pack so many elements"
-         " into one vector!");
-  if (MaxVectorSize.getFixedValue() == 0) {
+  if (MaxVectorSize.getKnownMinValue() == 0) {
     LLVM_DEBUG(dbgs() << "LV: The target has no vector registers.\n");
----------------
You could also do

  if (MaxVectorSize.isZero())


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98509