This is an archive of the discontinued LLVM Phabricator instance.

[Local] Fix a bug in the domtree update logic for MergeBasicBlockIntoOnlyPred.
ClosedPublic

Authored by bmakam on Oct 16 2017, 9:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

bmakam created this revision.Oct 16 2017, 9:40 AM
bmakam added a subscriber: llvm-commits.
kuhar edited edge metadata.Oct 16 2017, 9:46 AM

For some irreducible CFG the domtree nodes might be dead, do not update domtree for dead nodes.

Can you show some example of such CFG and add a comment explaining this check?

DomTree::getNode return nullptr only for forward-unreachable CFG nodes.

For some irreducible CFG the domtree nodes might be dead, do not update domtree for dead nodes.

Can you show some example of such CFG and add a comment explaining this check?

DomTree::getNode return nullptr only for forward-unreachable CFG nodes.

I found this bug while testing D37343. When D37343 is applied, test/Transforms/CodeGenPrepare/2008-11-24-RAUW-Self.ll will crash because of this.

kuhar added a comment.Oct 16 2017, 9:55 AM

I think that this deserves a comment with a (partial) example, otherwise the change looks suspicious.

I think that this deserves a comment with a (partial) example, otherwise the change looks suspicious.

Fair point. All the users of MergeBasicBlockIntoOnlyPred in the tree, do not pass DT to the function, so it is never exposed. Is there a way to test this in tree by passing DT to MergeBasicBlockIntoOnlyPred?

kuhar added a comment.EditedOct 16 2017, 10:09 AM

I think that this deserves a comment with a (partial) example, otherwise the change looks suspicious.

Fair point. All the users of MergeBasicBlockIntoOnlyPred in the tree, do not pass DT to the function, so it is never exposed. Is there a way to test this in tree by passing DT to MergeBasicBlockIntoOnlyPred?

It shouldn't be difficult to turn it into a unit test. I haven't checked how Local.cpp is tested, but you can find some existing dominator tree tests in IRTests/DominatorTree.cpp.

bmakam updated this revision to Diff 119222.Oct 16 2017, 3:50 PM

Add unittest and a comment.

kuhar added a comment.Oct 16 2017, 3:57 PM

Thanks for adding the comment.

The testcase seems really long and it's not obvious why what it tries to exercise -- have you tried reducing it with bugpoint to something shorter and easier to reason about?

Thanks for adding the comment.

The testcase seems really long and it's not obvious why what it tries to exercise -- have you tried reducing it with bugpoint to something shorter and easier to reason about?

Sorry for the long testcase, but this untitest was extracted after using bugpoint to reduce the lit test in test/Transforms/CodeGenPrepare/2008-11-24-RAUW-Self.ll. Is there a way to reduce a unittest using bugpoint?

bmakam updated this revision to Diff 119347.Oct 17 2017, 9:37 AM

Reduce unittest.

kuhar accepted this revision.Oct 23 2017, 11:42 AM

This reduced IR is much nicer.

This revision is now accepted and ready to land.Oct 23 2017, 11:42 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the reviews.