Page MenuHomePhabricator

[JumpThreading] Fix the AssertVH handling for debug builds.
Needs ReviewPublic

Authored by skatkov on Sep 4 2019, 9:24 PM.

Details

Summary

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

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

Diff Detail

Event Timeline

skatkov created this revision.Sep 4 2019, 9:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 9:24 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

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.

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.?

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.

Please also add a test-case showing the failure.

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.

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)

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