[JumpThreading] Fix the AssertVH handling for debug builds.

[skatkov]

Summary

A temporary created AssertVH should be destroyed before the Value is deleted.

The bug has been introduced in https://reviews.llvm.org/D44177.

[skatkov]

Note: I've got the crash by changing the DomTreeUpdater form Lazy mode to Eager. Even with this fix it will not work because of LVI->eraseBlock(&BB);
It seems the code is written in the way DTU cannot be Eager.

[efriedma]

It seems like there are multiple problems here if "BB" gets deleted eagerly: there the AssertingVH failure you're mentioning here, LVI->eraseBlock doesn't work, and the for loop "for (auto &BB : F)" also breaks. So I don't think this patch really solves anything.

Maybe it makes sense to change the TryToSimplifyUncondBranchFromEmptyBlock API so it doesn't erase the basic block, then call DeleteDeadBlock after we've cleaned up LVI etc.?

[brzycki]

Maybe it makes sense to change the TryToSimplifyUncondBranchFromEmptyBlock API so it doesn't erase the basic block, then call DeleteDeadBlock after we've cleaned up LVI etc.?

Depending on the number of call-sites of TryToSimplifyUncondBranchFromEmptyBlock this may be the way to go.

It seems the code is written in the way DTU cannot be Eager.

Originally the updates to DT were all immediate (Eager) and compile-time performance of the JumpThreading pass became considerably expensive. It's not uncommon for the pass to create BBs, modify the edges in/out of them, only to completely remove the code later when analysis at a different BB` shows the path is dead or unreachable.

The other, less-ideal, alternative is to always call LVI->eraseBlock(&BB) and simply lose the analysis for the block. It's no ideal but was actually the way this code behaved before Deferred DTU.

[brzycki]

Please also add a test-case showing the failure.

[skatkov]

Thank you guys, for feedback.

I found this issue fighting against the crash in jump-threading. It is in progress now and goes slowly due to intermittent behavior.
The current guess is related to broken DT and LoopInfo by jump threading pass but it uses alias analysis which uses Loop->getExitEdges(). So I tried to move from lazy update of DT to eager and found this issue, so posted the patch.
After that I found that it does not resolve the issue with eager DT as I wrote before.

I agree that this patch does not resolve the issue.
I will try to return to return to this one later when I have cycles. Functional crash has more priority for me now.
And I do not have clear reproducer at this moment.

So, thank you again and sorry if I bothered you.

[brzycki]

Hello @skatkov , I'm sorry to hear you're dealing with an intermittent crash. If you find a reproducible .ll file you can feed into opt -jump-threading -S < input.ll that crashes I would be interested in examining it. The Deferred usage of DTU causes unexpected issues and it is entirely possible there is still a bug here.

(CC'ing @kuhar and @NutshellySima to keep them informed)

[skatkov]

In D67207#1660225, @brzycki wrote:

Hello @skatkov , I'm sorry to hear you're dealing with an intermittent crash. If you find a reproducible .ll file you can feed into opt -jump-threading -S < input.ll that crashes I would be interested in examining it. The Deferred usage of DTU causes unexpected issues and it is entirely possible there is still a bug here.

(CC'ing @kuhar and @NutshellySima to keep them informed)

If you are interested in, I've filed a bug: https://bugs.llvm.org/show_bug.cgi?id=43276

llvm/lib/Transforms/Scalar/JumpThreading.cpp

for (auto &BB : F) {
       // ProcessBlock doesn't thread BBs with unconditional TIs. However, if BB
       // is "almost empty", we attempt to merge BB with its sole successor.
       auto *BI = dyn_cast<BranchInst>(BB.getTerminator());
       if (BI && BI->isUnconditional() &&
           // The terminator must be the only non-phi instruction in BB.
           BB.getFirstNonPHIOrDbg()->isTerminator() &&
           // Don't alter Loop headers and latches to ensure another pass can
           // detect and transform nested loops later.
-          !LoopHeaders.count(&BB) && !LoopHeaders.count(BI->getSuccessor(0)) &&
-          TryToSimplifyUncondBranchFromEmptyBlock(&BB, DTU)) {
+          !LoopHeaders.count(&BB) && !LoopHeaders.count(BI->getSuccessor(0)))
+        // This if should be separated from condition above because in debug
+        // build the key for LoopHeader is AssertVH. As a result in
+        // LoopHeaders.count check new temporary AssertVH will be created.
+        // At the same time the function TryToSimplifyUncondBranchFromEmptyBlock
+        // can remove the block causing the check for alive AssertVH will be
+        // failed and error will be triggered. So we need the temporary
+        // AssertVH is destroyed before TryToSimplifyUncondBranchFromEmptyBlock
+        // is invoked.
+        if (TryToSimplifyUncondBranchFromEmptyBlock(&BB, DTU)) {
           // BB is valid for cleanup here because we passed in DTU. F remains
           // BB's parent until a DTU->getDomTree() event.
           LVI->eraseBlock(&BB);
           Changed = true;
         }
     }
     EverChanged |= Changed;
   } while (Changed);
 
   LoopHeaders.clear();
   // Flush only the Dominator Tree.
   DTU->getDomTree();
   LVI->enableDT();