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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 5 inline comments as done.
fhahn 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(),
----------------
Ayal wrote:
> nit: would this look better?
Indeed, updated!


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


================
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);
----------------
Ayal wrote:
> nit: worth setting `auto FixedVF = VF->getKnownMinValue();`.
Updated, thanks!


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


================
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)) {
----------------
Ayal wrote:
> 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.
In theory there could be such expressions I think, but that particular one isn't handled incorrectly at the moment, possibly due to limitations in SCEV reasoning. Added additional tests in 01efcec6dbd1

But I checked compile-time impact of checking all expressions and there was no notable increase: https://llvm-compile-time-tracker.com/compare.php?from=9c1d65054818cd2fd9187cd7e7ef703d98b5c5e2&to=825bea6827d6558ab61c8e139f6d7ba4b007a69b&stat=instructions:u

updated the code to check all lanes in between as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148841