This is an archive of the discontinued LLVM Phabricator instance.

Fix bug to merge away entry block and update DT correctly.
AbandonedPublic

Authored by trentxintong on Jun 16 2018, 1:07 PM.

Details

Summary

Fix bug to merge away entry block and update DT correctly.

Diff Detail

Event Timeline

trentxintong created this revision.

run clang-format

Remove some duplicate code.

I admit I haven't looked at this code in a while, but I can't understand your fix. Can you please elaborate with an example?

lib/Transforms/Utils/Local.cpp
718

I'm a little confused by this comment. Isn't always the case that the entry block has no idom by definition of dominator tree?

unittests/Transforms/Utils/Local.cpp
220

The predecessors list here looks wrong?

trentxintong added inline comments.Jun 17 2018, 12:38 PM
lib/Transforms/Utils/Local.cpp
718

I should add "Entry block does not have IDom, do not attempt to getIDom() on it.
The problem with the code is we try to hook up the IDom of PredBB with dominator tree node of DestBB. But if PredBB is entry block, it does not have IDom , we need to recompute.

unittests/Transforms/Utils/Local.cpp
220

Yes, I will fix that.

Worth seeing D48202, which plans to remove this function in preference of MergeBlockIntoPredecessor (which does the merge the other way, so doesn't run into the same problem removing the entry block).

trentxintong abandoned this revision.Jun 17 2018, 1:18 PM

@dmgreen Thanks for the pointer. I think https://reviews.llvm.org/D48202 will define away the problem. I will abandon this patch.