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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2580-2589
+    // Build a new AddRec by multiplying the step by StepMultiplier and adding
+    // Offset * Step to the resulting AddRec.
+    auto *Ty = Expr->getType();
+    auto *StepC = SE.getConstant(Ty, StepMultiplier);
+    auto *OffsetC = SE.getConstant(Ty, Offset);
+    return SE.getAddExpr(
+        SE.getAddRecExpr(Expr->getStart(),
----------------
nit: would this look better?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2619
+
+  bool canAnalyze() const { return FoundUDiv && CanAnalyze; }
+};
----------------
nit: a bit discrepant for canAnalyze() to mean more than CanAnalyze. Perhaps the latter should be CannotAnalyze.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2640
+  // V uniform if the SCEV expressions are equal.
+  SCEVAddRecRewriter FirstLaneRewriter(*SE, VF->getKnownMinValue(), 0, TheLoop);
+  auto *FirstLaneExpr = FirstLaneRewriter.visit(S);
----------------
nit: worth setting `auto FixedVF = VF->getKnownMinValue();`.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2649
+  bool IsUniform =
+      LastLaneRewriter.canAnalyze() && FirstLaneExpr == LastLaneExpr;
+#ifndef NDEBUG
----------------
nit: suffice to set IsUniform to FirstLaneExpr == LastLaneExpr and assert that the latter is also canAnalyze if they're equal.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2652
+  if (IsUniform) {
+    // Make sure the expressions for all other lanes are also equal.
+    for (auto I : seq<unsigned>(1, VF->getKnownMinValue() - 1)) {
----------------
Ahh, could URem with "VF-1" lead to equal expressions for first and last lanes, but not for all lanes inbetween? E.g., along with FoundUDiv: ((i++)/2)%3) and VF=8.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148841