[GVNSink] Fix non-determinisms by using a deterministic ordering

[hiraditya]

GVNSink used to order instructions based on their pointer values and was prone to non-determinism because of that.
This patch ensures all the values stored are using a deterministic order. I have also added a verfier(ModelledPHI::verifyModelledPHI) to assert when ordering isn't preserved.

Additionally, I have added a test case that is a mirror image of an existing test.

Fixes: #77852

[ilovepi]

This seems like it should fix the issue to me, but lets see if @nikic agrees.

[efriedma-quic]

SplitBlockPredecessors invalidates the DFS numbering, I think.

Also, SplitBlockPredecessors changes the predecessor list of a basic block, which I think needs to invalidate any ModelledPHI referring to that block?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hiraditya]

SplitBlockPredecessors invalidates the DFS numbering, I think.

Makes sense to just update DFSNumbes with each split block.

Also, SplitBlockPredecessors changes the predecessor list of a basic block, which I think needs to invalidate any ModelledPHI referring to that block?

AFAICT all the ModelledPHI are created for a specific SinkBB (in SinkBB and analyzeInstructionForSinking functions) and then destroyed.

[efriedma-quic]

SplitBlockPredecessors invalidates the DFS numbering, I think.

Makes sense to just update DFSNumbes with each split block.

updateDFSNumbers walks the whole function, so that's quadratic in the size of the function. If you don't care about the exact numbers, maybe consider just building your own numbering as you visit PHI nodes. It's only a few lines to construct a DenseMap<BasicBlock*, int>.

Also, SplitBlockPredecessors changes the predecessor list of a basic block, which I think needs to invalidate any ModelledPHI referring to that block?

AFAICT all the ModelledPHI are created for a specific SinkBB (in SinkBB and analyzeInstructionForSinking functions) and then destroyed.

So the numbers don't need to be stable across blocks? That makes things simpler.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: AdityaK (hiraditya)

Changes

GVNSink used to order instructions based on their pointer values and was prone to non-determinism because of that.
This patch ensures all the values stored are using DFSNumber. I have also added a verfier(ModelledPHI::verifyModelledPHI) to assert when DFS ordering isn't preserved.

Additionally, I have added a test case that is a mirror image of an existing test.

Fixes: #77852

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/GVNSink.cpp (+69-16)
• (modified) llvm/test/Transforms/GVNSink/int_sideeffect.ll (+26)

diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp
index d4907326eb0a..0529c7e3be23 100644
--- a/llvm/lib/Transforms/Scalar/GVNSink.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp
@@ -42,6 +42,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/Analysis/DomTreeUpdater.h"
 #include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
@@ -226,12 +227,21 @@ class ModelledPHI {
 public:
   ModelledPHI() = default;
 
-  ModelledPHI(const PHINode *PN) {
-    // BasicBlock comes first so we sort by basic block pointer order, then by value pointer order.
-    SmallVector<std::pair<BasicBlock *, Value *>, 4> Ops;
+  ModelledPHI(const PHINode *PN, const DenseMap<const BasicBlock*, int64_t> &DFS) {
+    // BasicBlock comes first so we sort by basic block pointer order,
+    // then by value pointer order. No need to call `verifyModelledPHI`
+    // As the Values and Blocks are populated in DFSOrder already.
+    using OpsType = std::pair<BasicBlock *, Value *>;
+    SmallVector<OpsType, 4> Ops;
     for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I)
       Ops.push_back({PN->getIncomingBlock(I), PN->getIncomingValue(I)});
-    llvm::sort(Ops);
+
+    auto DFSOrder = [DFS](OpsType O1, OpsType O2) {
+      return DFS.lookup(O1.first) < DFS.lookup(O2.first);
+    };
+    // Sort by DFSNumber to have a deterministic order.
+    llvm::sort(Ops, DFSOrder);
+
     for (auto &P : Ops) {
       Blocks.push_back(P.first);
       Values.push_back(P.second);
@@ -247,16 +257,36 @@ class ModelledPHI {
     return M;
   }
 
+  void verifyModelledPHI(const DenseMap<const BasicBlock*, int64_t> &DFS) {
+    assert(Values.size() > 1 && Blocks.size() > 1 &&
+           "Modelling PHI with less than 2 values");
+    auto DFSOrder = [DFS](const BasicBlock *BB1, const BasicBlock *BB2) {
+      return DFS.lookup(BB1) < DFS.lookup(BB2);
+    };
+    assert(llvm::is_sorted(Blocks, DFSOrder));
+    int C = 0;
+    llvm::for_each(Values, [&C, this](const Value *V) {
+      if (!isa<UndefValue>(V)) {
+        const Instruction *I = cast<Instruction>(V);
+        assert(I->getParent() == this->Blocks[C]);
+      }
+      C++;
+    });
+  }
   /// Create a PHI from an array of incoming values and incoming blocks.
-  template <typename VArray, typename BArray>
-  ModelledPHI(const VArray &V, const BArray &B) {
+  ModelledPHI(SmallVectorImpl<Instruction *> &V,
+              SmallSetVector<BasicBlock *, 4> &B,
+              const DenseMap<const BasicBlock*, int64_t> &DFS) {
+    // The order of Values and Blocks are already as per their DFSNumbers.
     llvm::copy(V, std::back_inserter(Values));
     llvm::copy(B, std::back_inserter(Blocks));
+    verifyModelledPHI(DFS);
   }
 
   /// Create a PHI from [I[OpNum] for I in Insts].
-  template <typename BArray>
-  ModelledPHI(ArrayRef<Instruction *> Insts, unsigned OpNum, const BArray &B) {
+  /// TODO: Figure out a way to verifyModelledPHI in this constructor.
+  ModelledPHI(ArrayRef<Instruction *> Insts, unsigned OpNum,
+              SmallSetVector<BasicBlock *, 4> &B) {
     llvm::copy(B, std::back_inserter(Blocks));
     for (auto *I : Insts)
       Values.push_back(I->getOperand(OpNum));
@@ -297,7 +327,8 @@ class ModelledPHI {
 
   // Hash functor
   unsigned hash() const {
-      return (unsigned)hash_combine_range(Values.begin(), Values.end());
+    // Is deterministic because Values are saved in DFSOrder.
+    return (unsigned)hash_combine_range(Values.begin(), Values.end());
   }
 
   bool operator==(const ModelledPHI &Other) const {
@@ -566,7 +597,7 @@ class ValueTable {
 
 class GVNSink {
 public:
-  GVNSink() = default;
+  GVNSink(DominatorTree *DT) : DT(DT) {}
 
   bool run(Function &F) {
     LLVM_DEBUG(dbgs() << "GVNSink: running on function @" << F.getName()
@@ -575,6 +606,13 @@ class GVNSink {
     unsigned NumSunk = 0;
     ReversePostOrderTraversal<Function*> RPOT(&F);
     VN.setReachableBBs(BasicBlocksSet(RPOT.begin(), RPOT.end()));
+    // Populated DFSNumbers ahead of time to avoid updating dominator tree
+    // when CFG is modified. The DFSNumbers of newly created basic blocks
+    // are irrelevant because RPOT is also obtained ahead of time and only
+    // DFSNumbers of original CFG are relevant for sinkable candidates.
+    for (auto *BB: RPOT)
+      if (DT->getNode(BB))
+        DFSNumbers[BB] = DT->getNode(BB)->getDFSNumIn();
     for (auto *N : RPOT)
       NumSunk += sinkBB(N);
 
@@ -583,6 +621,8 @@ class GVNSink {
 
 private:
   ValueTable VN;
+  DominatorTree *DT;
+  DenseMap<const BasicBlock*, int64_t> DFSNumbers;
 
   bool shouldAvoidSinkingInstruction(Instruction *I) {
     // These instructions may change or break semantics if moved.
@@ -603,7 +643,7 @@ class GVNSink {
   void analyzeInitialPHIs(BasicBlock *BB, ModelledPHISet &PHIs,
                           SmallPtrSetImpl<Value *> &PHIContents) {
     for (PHINode &PN : BB->phis()) {
-      auto MPHI = ModelledPHI(&PN);
+      auto MPHI = ModelledPHI(&PN, DFSNumbers);
       PHIs.insert(MPHI);
       for (auto *V : MPHI.getValues())
         PHIContents.insert(V);
@@ -691,7 +731,7 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI,
   }
 
   // The sunk instruction's results.
-  ModelledPHI NewPHI(NewInsts, ActivePreds);
+  ModelledPHI NewPHI(NewInsts, ActivePreds, DFSNumbers);
 
   // Does sinking this instruction render previous PHIs redundant?
   if (NeededPHIs.erase(NewPHI))
@@ -766,6 +806,9 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) {
              BBEnd->printAsOperand(dbgs()); dbgs() << "\n");
   SmallVector<BasicBlock *, 4> Preds;
   for (auto *B : predecessors(BBEnd)) {
+    // Bailout on malformed CFG where BasicBlock has no predecessor(PR42346).
+    if (!DFSNumbers.count(B))
+      return 0;
     auto *T = B->getTerminator();
     if (isa<BranchInst>(T) || isa<SwitchInst>(T))
       Preds.push_back(B);
@@ -774,7 +817,11 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) {
   }
   if (Preds.size() < 2)
     return 0;
-  llvm::sort(Preds);
+  auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) {
+    return DFSNumbers.lookup(BB1) < DFSNumbers.lookup(BB2);
+  };
+  // Sort by DFSNumber to have a deterministic order.
+  llvm::sort(Preds, DFSOrder);
 
   unsigned NumOrigPreds = Preds.size();
   // We can only sink instructions through unconditional branches.
@@ -811,11 +858,12 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) {
   auto C = Candidates.front();
 
   LLVM_DEBUG(dbgs() << " -- Sinking: " << C << "\n");
+  DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager);
   BasicBlock *InsertBB = BBEnd;
   if (C.Blocks.size() < NumOrigPreds) {
     LLVM_DEBUG(dbgs() << " -- Splitting edge to ";
                BBEnd->printAsOperand(dbgs()); dbgs() << "\n");
-    InsertBB = SplitBlockPredecessors(BBEnd, C.Blocks, ".gvnsink.split");
+    InsertBB = SplitBlockPredecessors(BBEnd, C.Blocks, ".gvnsink.split", &DTU);
     if (!InsertBB) {
       LLVM_DEBUG(dbgs() << " -- FAILED to split edge!\n");
       // Edge couldn't be split.
@@ -886,8 +934,13 @@ void GVNSink::sinkLastInstruction(ArrayRef<BasicBlock *> Blocks,
 } // end anonymous namespace
 
 PreservedAnalyses GVNSinkPass::run(Function &F, FunctionAnalysisManager &AM) {
-  GVNSink G;
+  auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
+  GVNSink G(&DT);
+  DT.updateDFSNumbers();
   if (!G.run(F))
     return PreservedAnalyses::all();
-  return PreservedAnalyses::none();
+
+  PreservedAnalyses PA;
+  PA.preserve<DominatorTreeAnalysis>();
+  return PA;
 }
diff --git a/llvm/test/Transforms/GVNSink/int_sideeffect.ll b/llvm/test/Transforms/GVNSink/int_sideeffect.ll
index 3cc54e84f17c..9a3bc062dd94 100644
--- a/llvm/test/Transforms/GVNSink/int_sideeffect.ll
+++ b/llvm/test/Transforms/GVNSink/int_sideeffect.ll
@@ -28,3 +28,29 @@ if.end:
   ret float %phi
 }
 
+; CHECK-LABEL: scalarsSinkingReverse
+; CHECK-NOT: fmul
+; CHECK: = phi
+; CHECK: = fmul
+define float @scalarsSinkingReverse(float %d, float %m, float %a, i1 %cmp) {
+; This test is just a reverse(graph mirror) of the test
+; above to ensure GVNSink doesn't depend on the order of branches.
+entry:
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:
+  %add = fadd float %m, %a
+  %mul1 = fmul float %add, %d
+  br label %if.end
+
+if.else:
+  call void @llvm.sideeffect()
+  %sub = fsub float %m, %a
+  %mul0 = fmul float %sub, %d
+  br label %if.end
+
+if.end:
+  %phi = phi float [ %mul1, %if.then ], [ %mul0, %if.else ]
+  ret float %phi
+}
+

[efriedma-quic]

LGTM