[llvm] [GVNSink] Fix non-determinisms by using Depth-First ordering (PR #90995)

via llvm-commits llvm-commits at lists.llvm.org
Wed May 8 14:58:31 PDT 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: AdityaK (hiraditya)

<details>
<summary>Changes</summary>

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
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
+}
+

``````````

</details>


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


More information about the llvm-commits mailing list