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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn 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;
 
----------------
vporpo wrote:
> 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.
Thanks, extended the comment.


================
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.
----------------
vporpo wrote:
> A short description what this rewriter is for?
Added, thanks!


================
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) {
----------------
vporpo wrote:
> 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?
IIUC this should be an invariant, but I added the assertion to give us a chance to catch any violations and investigate. I think having this check only when assertions are enabled is in line with how assertions are used widely in the LLVM codebase (for better or worse). But if people prefer to err on the side of caution, we can always run the checks, at the cost of extra compile-time overhead.


================
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,
----------------
vporpo wrote:
> nit: Use seq<> ? `for (auto I : seq<unsigned>(1, VF->getKnownMinValue()-1))`
Thanks, updated!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148841