[IndVars] Fix strict weak ordering violation

[nikic]

The sort used the block name as a tie-breaker, which will not work for unnamed blocks and can result in a strict weak orering violation.

Fix this by checking that all exiting blocks dominate the latch first, which means that we have a total dominance order. This makes the code structure here align with what optimizeLoopExits() does.

Fixes #108618. Unfortunately I have not been actually able to reproduce the strict weak ordering assertion in a libcxx build, so this is a speculative fix based on reading the code. Thus also no test case. @alexfh Can you please confirm whether this really fixes the problem?

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

The sort used the block name as a tie-breaker, which will not work for unnamed blocks and can result in a strict weak orering violation.

Fix this by checking that all exiting blocks dominate the latch first, which means that we have a total dominance order. This makes the code structure here align with what optimizeLoopExits() does.

Fixes #108618. Unfortunately I have not been actually able to reproduce the strict weak ordering assertion in a libcxx build, so this is a speculative fix based on reading the code. Thus also no test case. @alexfh Can you please confirm whether this really fixes the problem?

---

Full diff: https://github.com/llvm/llvm-project/pull/108947.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/IndVarSimplify.cpp (+23-22)

diff --git a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
index 613597b0878814..53fe9df31babb7 100644
--- a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
+++ b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -1788,6 +1788,13 @@ bool IndVarSimplify::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
     return false;
   };
 
+  // Make sure all exits dominate the latch. This means there is a linear chain
+  // of exits. We check this before sorting so we have a total order.
+  BasicBlock *Latch = L->getLoopLatch();
+  for (BasicBlock *ExitingBB : ExitingBlocks)
+    if (!DT->dominates(ExitingBB, Latch))
+      return false;
+
   // If we have any exits which can't be predicated themselves, than we can't
   // predicate any exit which isn't guaranteed to execute before it.  Consider
   // two exits (a) and (b) which would both exit on the same iteration.  If we
@@ -1795,21 +1802,23 @@ bool IndVarSimplify::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
   // we could convert a loop from exiting through (a) to one exiting through
   // (b).  Note that this problem exists only for exits with the same exit
   // count, and we could be more aggressive when exit counts are known inequal.
-  llvm::sort(ExitingBlocks,
-            [&](BasicBlock *A, BasicBlock *B) {
-              // std::sort sorts in ascending order, so we want the inverse of
-              // the normal dominance relation, plus a tie breaker for blocks
-              // unordered by dominance.
-              if (DT->properlyDominates(A, B)) return true;
-              if (DT->properlyDominates(B, A)) return false;
-              return A->getName() < B->getName();
-            });
-  // Check to see if our exit blocks are a total order (i.e. a linear chain of
-  // exits before the backedge).  If they aren't, reasoning about reachability
-  // is complicated and we choose not to for now.
-  for (unsigned i = 1; i < ExitingBlocks.size(); i++)
-    if (!DT->dominates(ExitingBlocks[i-1], ExitingBlocks[i]))
+  llvm::sort(ExitingBlocks, [&](BasicBlock *A, BasicBlock *B) {
+    // std::sort sorts in ascending order, so we want the inverse of
+    // the normal dominance relation.
+    if (A == B)
+      return false;
+    if (DT->properlyDominates(A, B))
+      return true;
+    if (DT->properlyDominates(B, A))
       return false;
+    llvm_unreachable("Should have total dominance order");
+  });
+
+  // Make sure our exit blocks are really a total order (i.e. a linear chain of
+  // exits before the backedge).
+  for (unsigned i = 1; i < ExitingBlocks.size(); i++)
+    assert(DT->dominates(ExitingBlocks[i - 1], ExitingBlocks[i]) &&
+           "Not sorted by dominance");
 
   // Given our sorted total order, we know that exit[j] must be evaluated
   // after all exit[i] such j > i.
@@ -1822,14 +1831,6 @@ bool IndVarSimplify::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
   if (ExitingBlocks.empty())
     return false;
 
-  // We rely on not being able to reach an exiting block on a later iteration
-  // then it's statically compute exit count.  The implementaton of
-  // getExitCount currently has this invariant, but assert it here so that
-  // breakage is obvious if this ever changes..
-  assert(llvm::all_of(ExitingBlocks, [&](BasicBlock *ExitingBB) {
-        return DT->dominates(ExitingBB, L->getLoopLatch());
-      }));
-
   // At this point, ExitingBlocks consists of only those blocks which are
   // predicatable.  Given that, we know we have at least one exit we can
   // predicate if the loop is doesn't have side effects and doesn't have any

[nikic]

An alternative fix would have been to use the recently introduced block number as tie breaker instead. I thought it would be cleaner to do it this way though.

[alexfh]

Thanks! This fixes the strict weak ordering check failure on the inputs I tried.

[fhahn]

LGTM, thanks!