This is an archive of the discontinued LLVM Phabricator instance.

[LoopSimplifyCFG] Pay respect to LCSSA when removing dead blocks
ClosedPublic

Authored by mkazantsev on Jan 25 2019, 4:45 AM.

Details

Summary

Utility function that we use for blocks deletion always unconditionally removes
one-input Phis. In LoopSimplifyCFG, it can lead to breach of LCSSA form.
This patch alters this function to keep them if needed.

Diff Detail

Repository
rL LLVM

Event Timeline

mkazantsev created this revision.Jan 25 2019, 4:45 AM
fhahn added a subscriber: fhahn.Jan 25 2019, 4:59 AM

I think ideally we would just avoid deleting LCSSA PHI nodes (PHI nodes with a single incoming value in loop exit blocks), but maybe it is too much effort with the info available.

include/llvm/Transforms/Utils/BasicBlockUtils.h
44 ↗(On Diff #183518)

nit: How about KeepUselessPHIs, or KeepSingleValuePHIs, which is a bit more descriptive.

test/Transforms/LoopSimplifyCFG/lcssa.ll
2 ↗(On Diff #183518)

I think the relevant -verify-loop-lcssa & co are available without asserts as well, so we should be able to drop REQUIRE: asserts.

mkazantsev marked 2 inline comments as done.Jan 29 2019, 3:07 AM
mkazantsev added inline comments.
include/llvm/Transforms/Utils/BasicBlockUtils.h
44 ↗(On Diff #183518)

I used exactly the same name as existing function removePredecessor has. We can make a follow-up refactoring to change them all, but so far I'd prefer to keep things uniform.

test/Transforms/LoopSimplifyCFG/lcssa.ll
2 ↗(On Diff #183518)

I didn't check that. We can do it as follow-up if it works.

fedor.sergeev accepted this revision.Feb 11 2019, 5:33 AM

LGTM, but please, schedule those followups discussed here.

include/llvm/Transforms/Utils/BasicBlockUtils.h
44 ↗(On Diff #183518)

I also find this name very confusing...

This revision is now accepted and ready to land.Feb 11 2019, 5:33 AM
mkazantsev marked an inline comment as done.Feb 11 2019, 9:08 PM
mkazantsev added inline comments.
include/llvm/Transforms/Utils/BasicBlockUtils.h
44 ↗(On Diff #183518)

I will rename all its occurrences in a follow-up.

I'll split out changes in basic block utils as separate NFC, then submit change in LoopSimplifyCFG separately.

mkazantsev marked an inline comment as done.Feb 11 2019, 11:09 PM
mkazantsev added inline comments.
include/llvm/Transforms/Utils/BasicBlockUtils.h
44 ↗(On Diff #183518)
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 11:47 PM

I'll split out changes in basic block utils as separate NFC, then submit change in LoopSimplifyCFG separately.

Thanks, your effort is really appreciated!