Page MenuHomePhabricator

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

Authored by mkazantsev on Fri, Jan 25, 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.Fri, Jan 25, 4:45 AM
fhahn added a subscriber: fhahn.Fri, Jan 25, 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.Tue, Jan 29, 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.Mon, Feb 11, 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.Mon, Feb 11, 5:33 AM
mkazantsev marked an inline comment as done.Mon, Feb 11, 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.Mon, Feb 11, 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 TranscriptMon, Feb 11, 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!