[JumpThreading] Remove deleted BB from Unreachable

[weiguozhi]

Although an unreachable BB is skipped by processBlock, its successor can still be handled by processBlock, and maybeMergeBasicBlockIntoOnlyPred may merge the two BBs and delete the unreachable BB. Then the garbage pointer is left in Unreachable set. This patch removes the invalid BB pointer from the Unreachable set.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (weiguozhi)

Changes

Although an unreachable BB is skipped by processBlock, its successor can still be handled by processBlock, and maybeMergeBasicBlockIntoOnlyPred may merge the two BBs and delete the unreachable BB. Then the garbage pointer is left in Unreachable set. This patch removes the invalid BB pointer from the Unreachable set.

---

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

2 Files Affected:

• (modified) llvm/include/llvm/Transforms/Scalar/JumpThreading.h (+4)
• (modified) llvm/lib/Transforms/Scalar/JumpThreading.cpp (+5-3)

diff --git a/llvm/include/llvm/Transforms/Scalar/JumpThreading.h b/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
index 7d11fc0ad6938..84292c716a0a9 100644
--- a/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
+++ b/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
@@ -94,6 +94,10 @@ class JumpThreadingPass : public PassInfoMixin<JumpThreadingPass> {
   SmallPtrSet<const BasicBlock *, 16> LoopHeaders;
 #endif
 
+  // JumpThreading must not processes blocks unreachable from entry. It's a
+  // waste of compute time and can potentially lead to hangs.
+  SmallPtrSet<BasicBlock *, 16> Unreachable;
+
   unsigned BBDupThreshold;
   unsigned DefaultBBDupThreshold;
 
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 7b221a814aabd..dfa49ba183779 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -307,12 +307,11 @@ bool JumpThreadingPass::runImpl(Function &F_, FunctionAnalysisManager *FAM_,
   else
     BBDupThreshold = DefaultBBDupThreshold;
 
-  // JumpThreading must not processes blocks unreachable from entry. It's a
-  // waste of compute time and can potentially lead to hangs.
-  SmallPtrSet<BasicBlock *, 16> Unreachable;
   assert(DTU && "DTU isn't passed into JumpThreading before using it.");
   assert(DTU->hasDomTree() && "JumpThreading relies on DomTree to proceed.");
   DominatorTree &DT = DTU->getDomTree();
+
+  Unreachable.clear();
   for (auto &BB : *F)
     if (!DT.isReachableFromEntry(&BB))
       Unreachable.insert(&BB);
@@ -1902,6 +1901,9 @@ bool JumpThreadingPass::maybeMergeBasicBlockIntoOnlyPred(BasicBlock *BB) {
   LVI->eraseBlock(SinglePred);
   MergeBasicBlockIntoOnlyPred(BB, DTU.get());
 
+  if (Unreachable.count(SinglePred) && DTU->isBBPendingDeletion(SinglePred))
+    Unreachable.erase(SinglePred);
+
   // Now that BB is merged into SinglePred (i.e. SinglePred code followed by
   // BB code within one basic block `BB`), we need to invalidate the LVI
   // information associated with BB, because the LVI information need not be

[kazutakahirata]

Although an unreachable BB is skipped by processBlock, its successor can still be handled by processBlock, and maybeMergeBasicBlockIntoOnlyPred may merge the two BBs and delete the unreachable BB. Then the garbage pointer is left in Unreachable set. This patch removes the invalid BB pointer from the Unreachable set.

Ugh. I've encountered essentially the same problem elsewhere in LLVM (although I don't remember where). Meanwhile, in the inliner, when all call sites of a given static function have been inlined, we add the function to the list of dead functions, and then remove those dead functions after we are done inlining.

  if (Unreachable.count(SinglePred) && DTU->isBBPendingDeletion(SinglePred))
    Unreachable.erase(SinglePred);

I have a few questions about this:

• Is this the only place where a live basic block appears to be "Unreachable"?
• If DTU->isBBPendingDeletion(SinglePred) above is false, are we OK? Does somebody else remove SinglePred from Unrechable for us?
• Is it feasible to keep all basic blocks without erasing them? I'm guessing that this might be difficult because we sometimes call external utility functions to manipulate basic blocks, and those utility functions might remove basic blocks.

Thanks!

[nikic]

Can we bail out of maybeMergeBasicBlockIntoOnlyPred on unreachable pred instead?

[weiguozhi]

  if (Unreachable.count(SinglePred) && DTU->isBBPendingDeletion(SinglePred))
    Unreachable.erase(SinglePred);

I have a few questions about this:

• Is this the only place where a live basic block appears to be "Unreachable"?

A live basic block in Unreachable is not a problem. A deleted basic block in Unreachable is a problem. Here the SinglePred has not been deleted, but it is marked so in DTU. It will be actually deleted when we call DTU->flush() later.

• If DTU->isBBPendingDeletion(SinglePred) above is false, are we OK? Does somebody else remove SinglePred from Unrechable for us?

DTU->isBBPendingDeletion(SinglePred) is false means SinglePred will not be deleted, so it is OK for us. Current implementation never delete any pointer from Unrechable.

• Is it feasible to keep all basic blocks without erasing them? I'm guessing that this might be difficult because we sometimes call external utility functions to manipulate basic blocks, and those utility functions might remove basic blocks.

Maybe @nikic's suggestion can accomplish this. I will try it.

[weiguozhi]

Can we bail out of maybeMergeBasicBlockIntoOnlyPred on unreachable pred instead?

Done. But it causes regression in pr62908.ll.

[weiguozhi]

ping

[nikic]

Looking at this again, I'm not sure I fully understand what problem you're trying to solve here.

This code uses DTU so block deletion is deferred, so there are no problems with address reuse. Why is it problematic to merge into an unreachable block?

[nikic]

Can you please regenerate the test so the whole IR is visible?

[weiguozhi]

done.

[weiguozhi]

Looking at this again, I'm not sure I fully understand what problem you're trying to solve here.

This code uses DTU so block deletion is deferred, so there are no problems with address reuse. Why is it problematic to merge into an unreachable block?

As you have noticed MergeBasicBlockIntoOnlyPred can mark an unreachable block for deletion. The deletion is deferred until
DTU->flush() is called, then the unreachable BB is actually deleted, but the set Unreachable is not modified, so a deleted BB pointer is left in Unreachable.

In later optimizations new basic blocks can be generated, if we are unlucky a new BB may be created at the deleted BB address, then the new BB pointer is automatically contained in Unreachable unexpectedly. And the new BB will be skipped by the following optimization. This part is a random behavior, so we got indeterministic result on our real code.

[nikic]

As you have noticed MergeBasicBlockIntoOnlyPred can mark an unreachable block for deletion. The deletion is deferred until DTU->flush() is called, then the unreachable BB is actually deleted, but the set Unreachable is not modified, so a deleted BB pointer is left in Unreachable.

I think the last bit I'm still missing is when JumpThreading calls DTU->flush() during the transform. I thought JT only flushes DT updates after everything is done, at which point the Unreachable set is not relevant anymore.

[weiguozhi]

As you have noticed MergeBasicBlockIntoOnlyPred can mark an unreachable block for deletion. The deletion is deferred until DTU->flush() is called, then the unreachable BB is actually deleted, but the set Unreachable is not modified, so a deleted BB pointer is left in Unreachable.

I think the last bit I'm still missing is when JumpThreading calls DTU->flush() during the transform. I thought JT only flushes DT updates after everything is done, at which point the Unreachable set is not relevant anymore.

DTU->flush() is called in the chain:

    threadThroughTwoBasicBlocks()
->  getOrCreateBFI()
->  runExternalAnalysis<BlockFrequencyAnalysis>()
->  DTU->flush()

[nikic]

LGTM, thanks for the explanation. I think what JumpThreading does with BPI/BFI is unnecessarily complicated (why do we ever compute a fresh BPI/BFI if we're only doing updates?) and could maybe be cleaned up to avoid this flush, but let's land this fix for now.