This is an archive of the discontinued LLVM Phabricator instance.

[SimpleLoopUnswitch] Add non-empty unreachable block check to exit cases removed.
ClosedPublic

Authored by asbirlea on Apr 16 2020, 12:54 AM.

Details

Summary

Update check to include the check for unreachable.

Basic blocks ending in unreachable are special cased, as these blocks may be already unswitched. Before this patch this check is only done for the default destination.
The condition for the exit cases and the default case must be the same, because we should never leave edges from the switch instruction to a basic block that we are unswitching. In PR45355 we still have a remaining edge (that we're attempting to remove from the DT) because its the default edge to an unreachable-terminated block where we unswitch a case edge to that block.

Resolves PR45355.

Diff Detail

Event Timeline

asbirlea created this revision.Apr 16 2020, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 12:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Gentle ping.

Might also be good to explain a bit of *how* this fixes the PR to the commit log (or bug) for posterity. It seemed to surprise both of us that this was the fix when talking through the code, I suspect a future reader may benefit from having a log of what went on here.

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
623–625

Comment about the fact that the default and case conditions *must* be the exact same? Otherwise I fear we'll add back this bug some day soon...

asbirlea edited the summary of this revision. (Show Details)May 6 2020, 1:34 PM
asbirlea updated this revision to Diff 262466.May 6 2020, 1:35 PM
asbirlea marked an inline comment as done.

Address comment and updated description.

chandlerc accepted this revision.May 13 2020, 12:01 AM

LGTM!! Thanks for tracking down this surprising fix to the PR!

This revision is now accepted and ready to land.May 13 2020, 12:01 AM
This revision was automatically updated to reflect the committed changes.