[PATCH] D150851: [LoopVectorize] Vectorize select-cmp reduction pattern for increasing integer induction variable

[Ramkumar Ramachandra via Phabricator via llvm-commits]

artagnon added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:613-633
 // We are looking for loops that do something like this:
 //   int r = 0;
 //   for (int i = 0; i < n; i++) {
 //     if (src[i] > 3)
 //       r = 3;
 //   }
 // where the reduction value (r) only has two states, in this example 0 or 3.
----------------
Please update these comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:661
 
+  auto IsIncreasingLoopInduction = [&SE, &Loop](Value *V) {
+    auto *Phi = dyn_cast<PHINode>(V);
----------------
Why not split this out into its own function?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:663-671
+    if (!Phi)
+      return false;
+
+    if (!SE)
+      return false;
+
+    InductionDescriptor ID;
----------------
We should forbid these cases in the first place instead of checking them.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:674
+    const SCEVAddRecExpr *AR = cast<SCEVAddRecExpr>(SE->getSCEV(Phi));
+    if (!AR->hasNoSignedWrap())
+      return false;
----------------
Mel-Chen wrote:
> fhahn wrote:
> > Do we need to distinguish here between no signed/no unsigned wrap and then chose `smax/umax` during codegen?
> That's a good point. Implementing the patch for both signed and unsigned induction variables can be challenging in practice. 
> The current patch only focuses on the signed IV because in most applications, we often encounter induction variables in the form of IV {0, +, step}. When the IV is signed, we can use -1 or the minimum value of the signed data type as a sentinel value. However, when the IV is unsigned, we don't have a value smaller than 0 to use. This doesn't mean that unsigned IV cannot be vectorized, but rather they require additional handling and a more refined approach.
> 
> Of course, if an unsigned IV is {1, +, step}, we can directly use the method implemented in this patch. However, such cases are less common, so we decided to focus on handling signed IV first.
Doesn't the `InstDesc` tell you if it's signed or unsigned?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:677-679
+    ConstantInt *IVStartValue = dyn_cast<ConstantInt>(ID.getStartValue());
+    if (!IVStartValue || IVStartValue->isMinSignedValue())
+      return false;
----------------
Why is this necessary? Even if it isn't the min signed value, we should codegen just fine, as the codegen just involves applying a mask and applying max/min-reduce.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:681-682
+
+    const SCEV *Step = ID.getStep();
+    return SE->isKnownPositive(Step);
+  };
----------------
`getStepRecurrence()` from the SCEV AddRec, instead of relying on `isInductionPhi()`?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1089
+    // TODO: Decreasing induction need fix here
+    return Builder.CreateIntMaxReduce(Src, true);
+  default:
----------------
Personally, I prefer straight-line codegen as I've done, but I'm probably biased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150851