[PATCH] D75628: [DA] [SCEV] Provide facility to check for total ordering based on dominance

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:2566
   bool Disproved = false;
   if (testBounds(Dependence::DVEntry::ALL, 0, Bound, Delta)) {
     // Explore the direction vector hierarchy.
----------------
Is it even correct to call `testBounds` with SCEVExprs bounds with disjoint loop nests? Maybe the bug is in to consider `MaxLevels` instead of `CommonLevels`.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:2725
+  if (!SE->satisfiesTotalOrder(BoundExprs))
+    return true;
+  BoundExprs.clear();
----------------
Isn't `return false` the bail-out?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12364
+            [this](const Loop *LHS, const Loop *RHS) {
+              return this->DT.dominates(LHS->getHeader(), RHS->getHeader());
+            });
----------------
This is basically a 'contains' relationship. Correct?

Implementations of `std::sort` may assume that the relation is irreflexible (and `assert` on that). Use `properlyDominates` instead?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12369-12373
+  for (const Loop *L : SortedLoops) {
+    if (Prev && !DT.dominates(Prev->getHeader(), L->getHeader()))
+      return false;
+    Prev = L;
+  }
----------------
[suggestion]
```
for (int i = 1, e = SortedLoops.size(); i < e; ++i) {
  if (!DT.dominates(SortedLoops[i-1]->getHeader(), SortedLoops[i]->getHeader()))
    return false;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75628