This is an archive of the discontinued LLVM Phabricator instance.

[Dominators] Fix some edge cases for PostDomTree updating
ClosedPublic

Authored by dmgreen on Jan 18 2018, 9:59 AM.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 18 2018, 9:59 AM
kuhar accepted this revision.Jan 18 2018, 11:19 AM

Looks good overall, thanks for catching and fixing that.

unittests/IR/DominatorTreeBatchUpdatesTest.cpp
295

I think it's easier to read these tests when you order them by left nodes -- then it reads like IR, where for each BB you have some number of successors.
I'd even make sense to have them one line per each block, like:

1 3
3 4, 3 4
4 14
6 7, 6 9
...
This revision is now accepted and ready to land.Jan 18 2018, 11:19 AM
dmgreen updated this revision to Diff 130581.Jan 19 2018, 3:34 AM

Thanks. Nice idea.

I've updated the tests. I managed to manually simplify the longest one a bit too.

kuhar added a comment.Jan 19 2018, 8:34 AM

Thanks.

I think that this patch should be also merged into the 6.0 branch before the release.

This revision was automatically updated to reflect the committed changes.

Thanks.

No objections from me. I don't know of any way that current in-tree passes will trigger these, they mostly come from batch updating post dom trees, which is not something I think happens just yet. They should be safe to go into the branch, if you think thats best.

kuhar added a comment.Jan 20 2018, 6:51 AM

Thanks.

No objections from me. I don't know of any way that current in-tree passes will trigger these, they mostly come from batch updating post dom trees, which is not something I think happens just yet. They should be safe to go into the branch, if you think thats best.

ADCE uses the batch updater to update postdominators as well, ans it's used in 6.0.