This is an archive of the discontinued LLVM Phabricator instance.

DivergenceAnalysis: Fix crash with unreachable blocks
ClosedPublic

Authored by arsenm on Apr 28 2016, 12:28 AM.

Details

Summary

Unreachable blocks may not be in the dominator tree, so don't crash on them.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 55381.Apr 28 2016, 12:28 AM
arsenm retitled this revision from to DivergenceAnalysis: Fix crash with unreachable blocks.
arsenm updated this object.
arsenm added a reviewer: jingyue.
arsenm added a subscriber: llvm-commits.
jingyue requested changes to this revision.Apr 28 2016, 10:14 AM
jingyue edited edge metadata.

Thanks! I completely missed this case :(

lib/Analysis/DivergenceAnalysis.cpp
148

s/BBNode/ThisNode/

There are InfluenceBB and InfluenceNode below, so we'd better be explicit.

188

This seems unnecessary. ThisBB is guaranteed reachable according to the check you added above. InfluenceBB is guaranteed to dominate ThisBB, so it must be reachable too.

If you agree, maybe change this if to an assert.

This revision now requires changes to proceed.Apr 28 2016, 10:14 AM
arsenm added inline comments.Apr 28 2016, 10:25 AM
lib/Analysis/DivergenceAnalysis.cpp
188

I hit the first one, and then hit another crash here

jingyue added inline comments.Apr 28 2016, 10:45 AM
lib/Analysis/DivergenceAnalysis.cpp
188

Looking at your code more, the first check PDT.getNode doesn't really guard against unreachable basic blocks. You may want to use DT.getNode() or DT.isReachableFromEntry (I prefer the latter because it's more explicit) there. Then, I believe the second check here is unnecessary.

arsenm updated this revision to Diff 55459.Apr 28 2016, 12:38 PM
arsenm edited edge metadata.

Check isReachableFromEntry

jingyue accepted this revision.Apr 28 2016, 12:41 PM
jingyue edited edge metadata.
This revision is now accepted and ready to land.Apr 28 2016, 12:41 PM
arsenm closed this revision.Apr 28 2016, 11:23 PM

r268001