Page MenuHomePhabricator

[LoopUnrolling] Fix loop size check for peeling
ClosedPublic

Authored by mkazantsev on Mar 5 2017, 8:34 PM.

Details

Summary

We should check if loop size allows us to peel at least one iteration before we do so.

Diff Detail

Repository
rL LLVM

Event Timeline

mkazantsev created this revision.Mar 5 2017, 8:34 PM
mkazantsev updated this revision to Diff 90641.Mar 5 2017, 8:41 PM
mkuper accepted this revision.Mar 5 2017, 11:45 PM

LGTM with a style nit.

lib/Transforms/Utils/LoopUnrollPeel.cpp
78 ↗(On Diff #90641)

I would combine this into a single if with a &&, rather than two nested ones.

This revision is now accepted and ready to land.Mar 5 2017, 11:45 PM
mkazantsev added inline comments.Mar 6 2017, 2:23 AM
lib/Transforms/Utils/LoopUnrollPeel.cpp
78 ↗(On Diff #90641)

We can't combine it with variable's declaration, and declaring the latch externally looks quite ugly.

mkuper added inline comments.Mar 6 2017, 11:57 AM
lib/Transforms/Utils/LoopUnrollPeel.cpp
78 ↗(On Diff #90641)

I think having another level of nesting here is worse than declaring the latch externally.
I'd suggest either (1) declaring it externally, or (2) pull this entire thing into a helper function, in which case you can have an early exit.

mkazantsev added inline comments.Mar 6 2017, 7:19 PM
lib/Transforms/Utils/LoopUnrollPeel.cpp
78 ↗(On Diff #90641)

I just realized that getLoopPatch never returns null, and this is ensured within isLoopSimplifyForm in canPeel method. So I will replace the condition here and make an assert for latch.

mkazantsev updated this revision to Diff 90782.Mar 6 2017, 7:24 PM
mkazantsev marked 4 inline comments as done.
mkuper added a comment.Mar 6 2017, 7:29 PM

LGTM.
Also, forgot it before, sorry - please add a test. (No need for a huge test, you can force a small threshold.)

mkazantsev updated this revision to Diff 90784.Mar 6 2017, 7:30 PM
mkazantsev updated this revision to Diff 90787.Mar 6 2017, 8:01 PM

Do you need someone to check this in for you?

This revision was automatically updated to reflect the committed changes.