[BPI] Transfer value-handles when assign/move constructing BPI

[jmorse]

Background: BPI stores a collection of edge branch-probabilities, and also a set of Callback value-handles for the blocks in the edge-collection. When a block is deleted, BPI's eraseBlock method is called to clear the edge-collection of references to that block, to avoid dangling pointers.

However, when move-constructing or assigning a BPI object, the edge-collection gets moved, but the value-handles are discarded. This can lead to to stale entries in the edge-collection when blocks are deleted without the callback -- not normally a problem, but if a new block is allocated with the same address as an old block, spurious branch probabilities will be recorded about it. The fix is to transfer the handles from the source BPI object.

This was exposed by an unrelated debug-info change, it probably just shifted around allocation orders to expose this. Detected as nondeterminism and reduced by Zequan Wu:

f1b0a54#commitcomment-136737090

(No test because IMHO testing for a behaviour that varies with memory allocators is likely futile; I can add the reproducer with a CHECK for the relevant branch weights if it's desired though)

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Jeremy Morse (jmorse)

Changes

Background: BPI stores a collection of edge branch-probabilities, and also a set of Callback value-handles for the blocks in the edge-collection. When a block is deleted, BPI's eraseBlock method is called to clear the edge-collection of references to that block, to avoid dangling pointers.

However, when move-constructing or assigning a BPI object, the edge-collection gets moved, but the value-handles are discarded. This can lead to to stale entries in the edge-collection when blocks are deleted without the callback -- not normally a problem, but if a new block is allocated with the same address as an old block, spurious branch probabilities will be recorded about it. The fix is to transfer the handles from the source BPI object.

This was exposed by an unrelated debug-info change, it probably just shifted around allocation orders to expose this. Detected as nondeterminism and reduced by Zequan Wu:

f1b0a54#commitcomment-136737090

(No test because IMHO testing for a behaviour that varies with memory allocators is likely futile; I can add the reproducer with a CHECK for the relevant branch weights if it's desired though)

---

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

1 Files Affected:

• (modified) llvm/include/llvm/Analysis/BranchProbabilityInfo.h (+11-2)

diff --git a/llvm/include/llvm/Analysis/BranchProbabilityInfo.h b/llvm/include/llvm/Analysis/BranchProbabilityInfo.h
index 6b9d1781820111..91e1872e9bd6ff 100644
--- a/llvm/include/llvm/Analysis/BranchProbabilityInfo.h
+++ b/llvm/include/llvm/Analysis/BranchProbabilityInfo.h
@@ -122,16 +122,23 @@ class BranchProbabilityInfo {
   }
 
   BranchProbabilityInfo(BranchProbabilityInfo &&Arg)
-      : Probs(std::move(Arg.Probs)), LastF(Arg.LastF),
-        EstimatedBlockWeight(std::move(Arg.EstimatedBlockWeight)) {}
+      : Handles(std::move(Arg.Handles)), Probs(std::move(Arg.Probs)),
+        LastF(Arg.LastF),
+        EstimatedBlockWeight(std::move(Arg.EstimatedBlockWeight)) {
+    for (auto &Handle : Handles)
+      Handle.setBPI(this);
+  }
 
   BranchProbabilityInfo(const BranchProbabilityInfo &) = delete;
   BranchProbabilityInfo &operator=(const BranchProbabilityInfo &) = delete;
 
   BranchProbabilityInfo &operator=(BranchProbabilityInfo &&RHS) {
     releaseMemory();
+    Handles = std::move(RHS.Handles);
     Probs = std::move(RHS.Probs);
     EstimatedBlockWeight = std::move(RHS.EstimatedBlockWeight);
+    for (auto &Handle : Handles)
+      Handle.setBPI(this);
     return *this;
   }
 
@@ -279,6 +286,8 @@ class BranchProbabilityInfo {
     }
 
   public:
+    void setBPI(BranchProbabilityInfo *BPI) { this->BPI = BPI; }
+
     BasicBlockCallbackVH(const Value *V, BranchProbabilityInfo *BPI = nullptr)
         : CallbackVH(const_cast<Value *>(V)), BPI(BPI) {}
   };

[jmorse]

Thanks for the review; I'll merge shortly and file a cherry-pick request to the llvm-18 release branch, as I imagine most people want less nondeterminism in their compilers.

[MaskRay]

(No test because IMHO testing for a behaviour that varies with memory allocators is likely futile; I can add the reproducer with a CHECK for the relevant branch weights if it's desired though)

Agree that many times a test is not viable. Certain issues can be detected using a LLVM_ENABLE_REVERSE_ITERATION=on build.

We downstream check whether stage 2 and stage 3 clang executables are the same. This is a very heavy test.

Another way to check certain non-determinism problems is to replace the allocator.

These heavy changes probably rely on downstream testing and not build bots.