[PATCH] D148841: [LV] Use SCEV for uniformity analysis across VF

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:351
   /// Returns true if the value V is uniform within the loop.
-  bool isUniform(Value *V) const;
+  bool isUniform(Value *V, std::optional<ElementCount> VF = std::nullopt) const;
 
----------------
I think the description comment needs to be updated. It should mention that can now check uniformity within the VF, which should explain why we need the VF argument.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2550
+namespace {
+class SCEVAddRecRewriter : public SCEVRewriteVisitor<SCEVAddRecRewriter> {
+  /// Multiplier to be applied to the step of AddRecs in TheLoop.
----------------
A short description what this rewriter is for?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2641
+  if (IsUniform) {
+    // Make sure the expressions for all other lanes are also equal.
+    for (unsigned I = 1; I < VF->getKnownMinValue() - 1; ++I) {
----------------
Can't think of any case where this would trigger, but if there is such a corner case we may still miss it if it is only checked in the debug build, silently causing miscompilations. So perhaps we should not guard it under NDEBUG and use an llvm_unreachable instead of an assertion so that it is also checked in release builds? Wdyt?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2642
+    // Make sure the expressions for all other lanes are also equal.
+    for (unsigned I = 1; I < VF->getKnownMinValue() - 1; ++I) {
+      SCEVAddRecRewriter IthLaneRewriter(*SE, VF->getKnownMinValue(), I,
----------------
nit: Use seq<> ? `for (auto I : seq<unsigned>(1, VF->getKnownMinValue()-1))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148841