This is an archive of the discontinued LLVM Phabricator instance.

[LoopPeel] Re-factor llvm::peelLoop method. NFC.
ClosedPublic

Authored by skatkov on Jun 27 2019, 11:52 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

skatkov created this revision.Jun 27 2019, 11:52 PM

LGTM, just 2 small nits.

lib/Transforms/Utils/LoopUnrollPeel.cpp
390 ↗(On Diff #206996)

Using weight in the description for ExitWeight and # of times for CurHeaderWeigth seems inconsistent? (Also in some comments)

397 ↗(On Diff #206996)

nit: no brackets required?

reames accepted this revision.Jun 28 2019, 3:13 PM

Marking as accepted per previous reviewer (just so that search shows it as such)

This revision is now accepted and ready to land.Jun 28 2019, 3:13 PM
skatkov marked 2 inline comments as done.Jun 30 2019, 11:46 PM
skatkov added inline comments.
lib/Transforms/Utils/LoopUnrollPeel.cpp
390 ↗(On Diff #206996)

Could you please elaborate a bit on this comment?
I did not change anything in the semantic, just moved the code in a separate functions.

So the weights still make sense...

397 ↗(On Diff #206996)

Sure, will remove before landing.

Hi Florian, I will land this change with comment update because I do not understand what exactly comment needs to be fixed.
Feel free to update this review with details what wrong in comment you see and I will land it as follow-up patch.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 10:59 PM
fhahn added inline comments.Jul 3 2019, 4:53 AM
lib/Transforms/Utils/LoopUnrollPeel.cpp
390 ↗(On Diff #206996)

Yep, it was = a small thing. I just found it a bit inconsistent that ExitWeight and CurHeaderWeight are described differently. I think something like \param[out] ExitWeight The number of times the edge from Latch to Exit block is executed (or taken) would be more consistent with the description for CurHeaderWeight.

skatkov marked an inline comment as done.Jul 3 2019, 10:10 PM