This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Mark missed Changed to true.
ClosedPublic

Authored by asbirlea on Jul 30 2019, 4:59 PM.

Details

Summary

DominatorTree is invalid after SimplifyCFG because of a missed Changed = true when simplifying a branch condition and removing an edge.
Resolves PR42272.

Diff Detail

Event Timeline

asbirlea created this revision.Jul 30 2019, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 4:59 PM

*ping* We would like to bump up the priority of this since this allows us to enable the new PM when building fuchsia.

sanjoy accepted this revision.Jul 31 2019, 5:57 PM
sanjoy added a subscriber: sanjoy.

lgtm

lib/Transforms/Utils/SimplifyCFG.cpp
4181

Is this check necessary? If not, might be nice to remove it (in a separate change).

4192

You could sink down the Changed = true; outside the if/else.

test/Transforms/SimplifyCFG/invalidate-dom.ll
16

You can use opt -metarenamer to give names to all these instructions. That makes the test a bit more resilient to changes.

This revision is now accepted and ready to land.Jul 31 2019, 5:57 PM
asbirlea updated this revision to Diff 212863.Aug 1 2019, 11:28 AM
asbirlea marked 4 inline comments as done.

Address comments.

lib/Transforms/Utils/SimplifyCFG.cpp
4192

Right, but this is non-obvious because of the else if below (instead of else). Unless one notices that this iterates over BB's predecessors, then they may think BB may not be either the 0 or 1 successor.
Let me send the clean-up patch for the above and for this separately, since we can also sink the call to EraseTerminatorAndDCECond(BI); here.

test/Transforms/SimplifyCFG/invalidate-dom.ll
16

SG, but I renamed just the instructions manually. The metarenamer renames everything and I for one get more confused when everything is named foo, bar etc :).

This revision was automatically updated to reflect the committed changes.