[llvm] [CloneFunction] Fix non-deterministic PHI cleanup using PHINode::removeIncomingValueIf() (PR #173975)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Mingjie Xu (Enna1)

<details>
<summary>Changes</summary>

Previously, we use `std::map<BasicBlock *, unsigned> PredCount` to track excess incoming blocks and removed them one by one using `removeIncomingValue`. then we remove the excess incoming blocks one by one.

Since `PredCount` use `BasicBlock *` as key, the iteration order depends on the memory addresses of the blocks. With `PHINode::removeIncomingValue()` changed to use the swapping strategy, the order in which operands are removed affects the final order of the remaining operands in the PHI node. This will cause non-determinism in compiles.

This patch uses `PHINode::removeIncomingValueIf()` to remove invalid incoming blocks that no longer go to `NewBB` block, fixes the non-determinism.

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


1 Files Affected:

- (modified) llvm/lib/Transforms/Utils/CloneFunction.cpp (+9-16) 


``````````diff
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 1d0244c54a282..8ccbc33915d06 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -810,24 +810,17 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
     NumPreds = pred_size(NewBB);
     if (NumPreds != PN->getNumIncomingValues()) {
       assert(NumPreds < PN->getNumIncomingValues());
-      // Count how many times each predecessor comes to this block.
-      std::map<BasicBlock *, unsigned> PredCount;
-      for (BasicBlock *Pred : predecessors(NewBB))
-        --PredCount[Pred];
-
-      // Figure out how many entries to remove from each PHI.
-      for (BasicBlock *Pred : PN->blocks())
-        ++PredCount[Pred];
-
-      // At this point, the excess predecessor entries are positive in the
-      // map.  Loop over all of the PHIs and remove excess predecessor
-      // entries.
+      SmallPtrSet<BasicBlock *, 4> NewBBPreds(llvm::from_range,
+                                              predecessors(NewBB));
+
       BasicBlock::iterator I = NewBB->begin();
       for (; (PN = dyn_cast<PHINode>(I)); ++I) {
-        for (const auto &[Pred, Count] : PredCount) {
-          for ([[maybe_unused]] unsigned _ : llvm::seq<unsigned>(Count))
-            PN->removeIncomingValue(Pred, false);
-        }
+        PN->removeIncomingValueIf(
+            [&](unsigned Idx) {
+              BasicBlock *IncomingBlock = PN->getIncomingBlock(Idx);
+              return !NewBBPreds.contains(IncomingBlock);
+            },
+            false);
       }
     }
 

``````````

</details>


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