Page MenuHomePhabricator

Move deleteDeadLoop to LoopUtils. NFC
ClosedPublic

Authored by kariddi on Oct 2 2017, 9:50 AM.

Details

Summary

Moving deleteDeadLoop in LoopUtils like the FIXME in the comments is asking to be done.

I made it such that can update various analysis conditionally in case we are running it in a pass that doesn't preserve those analysis anyway

Diff Detail

Repository
rL LLVM

Event Timeline

kariddi created this revision.Oct 2 2017, 9:50 AM
anna added inline comments.Oct 3 2017, 12:25 PM
include/llvm/Transforms/Utils/LoopUtils.h
444

These comments are stale now. Please update.

lib/Transforms/Utils/LoopUtils.cpp
1142

These comments are stale now. The 2 kinds of "dead" loops were specific to loop deletion.

1147

Please state as a comment that we also have certain requirements on the structure of the loop and the deletion works by connecting the preheader directly to the exit. (these conditions have been specified as asserts below: preheader has exactly one successors, we have exactly one exit block etc).

kariddi updated this revision to Diff 117576.Oct 3 2017, 1:38 PM

Thanks Anna for the review.

I modified the comments

anna added inline comments.Oct 4 2017, 7:31 AM
lib/Transforms/Utils/LoopUtils.cpp
1145

This second part is incorrect. We can have any number of predecessors to the exit block, we just remove all the predecessors (because these are from the dead loop) and set the preheader as the only predecessor of the exit block.

Pls rmove the second part of the statement.

kariddi added inline comments.Oct 4 2017, 8:55 AM
lib/Transforms/Utils/LoopUtils.cpp
1145

What I meant with that is that the predecessors of the exit need to be only blocks from the loop (aka. The loop has dedicated exits).

That should be a requirement for the function (There's an assert for that). I'll reword it by just saying that the exit needs to be dedicated.
I agree that how it is right now is confusing

kariddi updated this revision to Diff 117679.Oct 4 2017, 8:58 AM

Updated confusing comment after Anna's suggestion.

anna accepted this revision.Oct 4 2017, 9:31 AM

LGTM with nits.

lib/Transforms/Utils/LoopUtils.cpp
1145

Could you phrase it as "a unique dedicated exit block must exist".

Btw, please keep this explanation in the LoopUtils header. We don't need to repeat it here. I think that's the convention we follow in llvm header and cpp files, to avoid repetition.

This revision is now accepted and ready to land.Oct 4 2017, 9:31 AM

Thanks Anna.
Addressed your nitpicks.

Forgot to add the Differential revision in the commit message, so I'll close this manually as it will not do it by itself.

kariddi closed this revision.Oct 4 2017, 1:45 PM