[PATCH] D146199: [LoopVectorize] Don't tail-fold for scalable VFs when there is no scalar tail

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5155
+  // we choose.
+  unsigned MaxPossibleVF = 1;
+  if (MaxFactors.FixedVF.isVector())
----------------
Is `MaxRuntimeVF` a better name?

Also, is it worth making this a `std::optional<unsigned>`? I struggled to wrap my head around what `MaxPossibleVF = 1` meant, because I assumed this meant "don't vectorize, use scalar instructions", given other uses of `VF=1` in the LV.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5158
+    MaxPossibleVF = MaxFactors.FixedVF.getFixedValue();
+  if (MaxFactors.ScalableVF) {
+    // If vscale is a power of 2 then we only need to look at the maximum
----------------
I initially wanted to ask "should this be `else if (MaxFactors.ScalableVF) {`?" but then I realised we need to be conservative here because the decision (not) to do tail folding is made for _both_ fixed and scalable VFs. So should this code be considering the `max(MaxFactors.FixedVF, MaxVScale * MaxFactors.ScalableVF)` instead?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5165-5166
+    if (TTI.isVScaleKnownToBeAPowerOfTwo()) {
+      std::optional<unsigned> MaxVScale = getMaxVScale(*TheFunction, TTI);
+      if (MaxVScale) {
+        unsigned MaxPossibleScalableVF =
----------------
nit: move assignment into the condition?

  if (std::optional<unsigned> MaxVScale = getMaxVScale(*TheFunction, TTI)) {
    ..
  }


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5173
+    } else
+      MaxPossibleVF = 1; // Stick with tail-folding for now.
+  }
----------------
nit: This default is already set on line 5155, so there's no need to set it again here?


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

https://reviews.llvm.org/D146199