[PATCH] D90272: [BranchProbabilityInfo] Fix eraseBlock

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for figuring out the root cause!

> It replaces the terminator of A with UnreachableInst before (indirectly) calling eraseBlock(A).

Question: Why is it UnreachableInst?



================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:1141
+
+  if (MaxSuccIdx.find(Src) == MaxSuccIdx.end())
+    MaxSuccIdx[Src] = IndexInSuccessors;
----------------
wmi wrote:
> If Src doesn't exist in MaxSuccIdx, MaxSuccIdx[Src] will return 0. Can it be simplified to one statement?
> MaxSuccIdx[Src] = std::max(MaxSuccIdx[Src], IndexInSuccessors);
This can be simply: `int &SuccIdx = MaxSuccIdx[Src]; SuccIdx = std::max(SuccIdx, IndexInSuccessors);`

Alternatively,

try_emplace + std::max


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:1182
 void BranchProbabilityInfo::eraseBlock(const BasicBlock *BB) {
-  for (const_succ_iterator I = succ_begin(BB), E = succ_end(BB); I != E; ++I) {
-    auto MapI = Probs.find(std::make_pair(BB, I.getSuccessorIndex()));
+  auto It = MaxSuccIdx.find(BB);
+  if (It == MaxSuccIdx.end())
----------------
Add a comment that we cannot simply use successors?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90272/new/

https://reviews.llvm.org/D90272