[PATCH] D142894: [LoopVectorize] Use overflow-check analysis to improve tail-folding.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1572-1579
+    TailFoldingStyle Style = TTI.getPreferredTailFoldingStyle();
+    // If we know the runtime check will be false, it's more efficient to use
+    // the DataAndControlFlow with the runtime check, because that avoids
+    // using a cmp/sel for the modified tripcount.
+    if (Style == TailFoldingStyle::DataAndControlFlowWithoutRuntimeCheck &&
+        IVUpdateCannotOverflow)
+      return TailFoldingStyle::DataAndControlFlow;
----------------
It seems weird to ask the target for their preferred style and then immediate override it.  Is there a reason not to pass `IVUpdateCannotOverflow` to `getPreferredTailFoldingStyle`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3039
 
-  TailFoldingStyle Style = Cost->getTailFoldingStyle();
+  bool IVUpdateCannotOverflow = Cost->isIndvarOverflowCheckKnownFalse(VF, UF);
+  TailFoldingStyle Style = Cost->getTailFoldingStyle(IVUpdateCannotOverflow);
----------------
Up to you but personally I think for all instances the reverse polarity `IVUpdateCanOverflow` reads better.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3483
 
+bool LoopVectorizationCostModel::isIndvarOverflowCheckKnownFalse(
+    ElementCount VF, std::optional<unsigned> UF) const {
----------------
Forgive my ignorance here but this doesn't look like a cost model question?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3495
+  // We know the runtime overflow check is known false iff the (max) trip-count
+  // is known and (max) trip-count + VF does not overflow in the type of the
+  // vector loop induction variable.
----------------
`num-elts-per-iteration` or `(VF * UF)`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3497
+  // vector loop induction variable.
+  if (std::optional<unsigned> TC = getSmallBestKnownTC(*PSE.getSE(), TheLoop)) {
+    unsigned MaxVFElts = VF.getKnownMinValue();
----------------
david-arm wrote:
> I wonder if this is safe because `getSmallBestKnownTC` also returns a value from profiling for the expected trip count, which is more like a hint rather than a definite?
This doesn't look entirely safe.  I think you really want `SE.getSmallConstantTripCount`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3498
+  if (std::optional<unsigned> TC = getSmallBestKnownTC(*PSE.getSE(), TheLoop)) {
+    unsigned MaxVFElts = VF.getKnownMinValue();
+    if (VF.isScalable()) {
----------------
`MaxVF`? because Elts is implied.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3500
+    if (VF.isScalable()) {
+      if (std::optional<unsigned> MaxVScale = TTI.getMaxVScale())
+        MaxVFElts *= *MaxVScale;
----------------
We so need to get rid of `TTI::getMaxVScale()`.  I think function attributes should take precedence but then this is the order used within `getMaxLegalScalableVF` so perhaps best fixed separately. Up to you.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3503
+      else if (TheFunction->hasFnAttribute(Attribute::VScaleRange))
+        MaxVFElts *= *TheFunction->getFnAttribute(Attribute::VScaleRange)
+                         .getVScaleRangeMax();
----------------
I suppose this can overflow. Perhaps make `MaxVFElts` an `uint64_t` given that's what `ugt` will promote to anyway.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8970
+       ElementCount::isKnownLT(VF, Range.End); VF *= 2)
+    IVUpdateCannotOverflow &= CM.isIndvarOverflowCheckKnownFalse(VF);
+
----------------
Is `addCanonicalIVRecipes` being passed the "wrong" tail folding style a functional issue? or just a corner case that might affect performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142894