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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour 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.
----------------
Meinersbur wrote:
> 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`.
The Banerjee Inequality requires that delta between the coefficients of the first terms of the Diophantine function `h` be in between LB and UB, where LB and UB are defined to be the sum of min/max quantities over **all** loop levels (not just the common levels), so the code is correct in looping over all loops including the siblings. This is more of a limitation in SCEV not being able to represent the bounds when they are non-invariant recurrences that straddle non-dominating loops.

I'm actually not quite sure why there is this assumption in SCEV about dominance. Do you know?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:2725
+  if (!SE->satisfiesTotalOrder(BoundExprs))
+    return true;
+  BoundExprs.clear();
----------------
Meinersbur wrote:
> Isn't `return false` the bail-out?
No, this function is trying to disprove dependence by returning false. If the delta is known not to be in the range of [LB, UB] then it returns false  meaning that there is no real (or integer) solution to the diophantine equation which indicates that there is no dependence. When we cannot prove that the delta is in the range, then the function conservatively returns true (meaning that the dependence cannot be disproved).


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12364
+            [this](const Loop *LHS, const Loop *RHS) {
+              return this->DT.dominates(LHS->getHeader(), RHS->getHeader());
+            });
----------------
Meinersbur wrote:
> 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?
Right, it's a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75628