[PATCH] D147735: [LV] Adds simple analysis that improves VF-aware uniformity checks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for sharing the patch! I added some comments inline. Reasoning about this manually seems a bit fragile and verbose so I was wondering if it might be possible to re-use the existing reasoning infrastructure we have in SCEV to do the heavy lifting. I put up a patch that use SCEV, but I might be missing something that would make this unfeasible: D148841 <https://reviews.llvm.org/D148841>



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:488
+  auto *IV = TheLoop->getInductionVariable(SE);
+  if(IV == nullptr)
+    return false;
----------------
This seems overly restrictive.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:495
+      auto Opcode = cast<Instruction>(V)->getOpcode();
+      if (Opcode == Instruction::UDiv && isInductionVariable(Op1)) {
+        // Return true if divisor/step is a constant and a multiple of VF.
----------------
Shouldn't this check if `Op1` is the induction we will use the step from?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:502
+          std::optional<Loop::LoopBounds> LoopBounds =
+              Loop::LoopBounds::getBounds(*TheLoop, *IV, SE);
+          assert(LoopBounds && "Missing LoopBounds!");
----------------
Using this API to just get the step value seems unnecessary restrictive. Better to just get the induction descriptor for the induction.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:503
+              Loop::LoopBounds::getBounds(*TheLoop, *IV, SE);
+          assert(LoopBounds && "Missing LoopBounds!");
+          ConstantInt *Step = dyn_cast<ConstantInt>(LoopBounds->getStepValue());
----------------
IIUC `getBounds` will only work if `IV` is the main induction phi, but there could be multiple inductions.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:506
+          // (Divisor / Step) % VF == 0
+          return Step != nullptr && !IsIdentity &&
+                 Divisor->getValue()
----------------
I might be missing where this is handled, but doesn't the code also have to account for the start value of the induction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147735