Page MenuHomePhabricator

[LoopUnroll] Don't modify TripCount/TripMultiple in computeUnrollCount()
ClosedPublic

Authored by nikic on Jun 19 2021, 1:20 AM.

Details

Summary

Minor cleanup: As these are no longer passed to UnrollLoop(), there is no need to modify them in computeUnrollCount(). Make them non-reference parameters.

Diff Detail

Event Timeline

nikic created this revision.Jun 19 2021, 1:20 AM
nikic requested review of this revision.Jun 19 2021, 1:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2021, 1:20 AM
nikic added inline comments.Jun 19 2021, 1:28 AM
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
770

It would be nice to get rid of UseUpperBound as well, but apparently this is still being used by LoopUnrollAndJam in https://github.com/llvm/llvm-project/blob/876de062f94650f9ded56a22b062236f711fcd18/llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp#L177. No test coverage though :(

reames added inline comments.Jun 19 2021, 10:02 AM
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
770

Ouch, this code is all around terrible isn't it?

1164–1165

I may be missing something, but I think you're changing behavior here. computeUnrollCount appears to indirectly control ULO.Count via this line. Unless I'm misreading this? If I'm not, I'm surprised tests don't cover this.

nikic updated this revision to Diff 353205.Jun 19 2021, 11:11 AM

Remove obsolete unroll count clamp.

nikic added inline comments.Jun 19 2021, 11:14 AM
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
1164–1165

I think this is just dead code and have dropped it. UnrollLoop() will clamp the unroll count to the max trip count itself, so this is redundant.

Note that TripCount and TripMultiple are also used in the UP.Runtime adjustment below, but computeUnrollCount() only changes TripCount and TripMultiple when fully unrolling, so it shouldn't matter for runtime unrolling.

nikic updated this revision to Diff 353207.Jun 19 2021, 12:31 PM

Remove outdated comment.

reames accepted this revision.Jun 21 2021, 9:38 AM

LGTM

Reading through the code again, I think I agree with your analysis.

This revision is now accepted and ready to land.Jun 21 2021, 9:38 AM
This revision was landed with ongoing or failed builds.Jun 21 2021, 12:34 PM
This revision was automatically updated to reflect the committed changes.