Page MenuHomePhabricator

[unroll] Split full exact and full bound unroll costing [NFC]
ClosedPublic

Authored by reames on Tue, Nov 23, 9:41 AM.

Details

Summary

This change should be NFC. It's posted for review mostly to make sure others are happy with the names I'm introducing for "exact full unroll" and "bounded full unroll". The motivation here is that our cost model for bounded unrolling is too aggressive - it gives benefits for exits we aren't going to prune - but I also just think the new version of the code is a lot easier to follow.

Diff Detail

Event Timeline

reames created this revision.Tue, Nov 23, 9:41 AM
reames requested review of this revision.Tue, Nov 23, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 23, 9:41 AM
reames updated this revision to Diff 389240.Tue, Nov 23, 9:47 AM

Further simplify - despite structure of existing code, shouldFullUnroll doesn't modify it's UP param.

I'm not sure i'm ready to comment on that whole logic, or whether the change is NFC, but the names don't seem particularly wrong there.

fhahn accepted this revision.Thu, Nov 25, 3:21 AM

LGTM, thanks!

llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
957

nit: it might also be a good opportunity to limit the scope of UnrollFactor to this block, e.g. by if (auto UnrollFactor = shouldFullUnroll(L, TTI, DT, SE, EphValues, TripCount, UCE, UP)) {}

This revision is now accepted and ready to land.Thu, Nov 25, 3:21 AM
nikic added inline comments.Thu, Nov 25, 9:35 AM
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
951

All copies of some exit test, not necessarily the latch exit test.

This revision was landed with ongoing or failed builds.Mon, Nov 29, 2:19 PM
This revision was automatically updated to reflect the committed changes.