This is an archive of the discontinued LLVM Phabricator instance.

[LICM] Report successful hoists/sinks
ClosedPublic

Authored by anemet on Dec 19 2016, 1:34 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

anemet updated this revision to Diff 81992.Dec 19 2016, 1:34 PM
anemet retitled this revision from to [LICM] Report successful hoists/sinks.
anemet updated this object.
anemet added a reviewer: hfinkel.
anemet added a subscriber: llvm-commits.
sanjoy added a comment.Jan 3 2017, 9:46 AM

How about adding remarks to promoteLoopAccessesToScalars? Or did you intend to do that later?

davide requested changes to this revision.Jan 3 2017, 9:50 AM
davide added a reviewer: davide.
davide added a subscriber: davide.

I think this needs more tests (a suggestion inline).

test/Transforms/LICM/opt-remarks.ll
11–16 ↗(On Diff #81992)

This test is fine but you should add another one where you show up that the remark is emitted when a load is sunk

This revision now requires changes to proceed.Jan 3 2017, 9:50 AM
anemet added inline comments.Jan 3 2017, 10:04 AM
test/Transforms/LICM/opt-remarks.ll
11–16 ↗(On Diff #81992)

You mean, store, right?

davide added inline comments.Jan 3 2017, 10:05 AM
test/Transforms/LICM/opt-remarks.ll
11–16 ↗(On Diff #81992)

yeah, fat fingering =)

How about adding remarks to promoteLoopAccessesToScalars? Or did you intend to do that later?

Sure, I can add them to that too.

anemet updated this revision to Diff 83101.Jan 4 2017, 11:51 AM
anemet edited edge metadata.

Address both Sanjoy's and Davide's comments.

davide accepted this revision.Jan 6 2017, 6:29 PM
davide edited edge metadata.

LGTM

lib/Transforms/Scalar/LICM.cpp
220–224 ↗(On Diff #83101)

runOnLoop and friends now take 8 arguments. I think it's a bit too much. Not your fault but clearly a sign that LICM structure could use some love.

This revision is now accepted and ready to land.Jan 6 2017, 6:29 PM
davide added inline comments.Jan 6 2017, 6:31 PM
include/llvm/Transforms/Utils/LoopUtils.h
407–412 ↗(On Diff #83101)

This comment needs to be updated to reflect the fact we now also take an ORE

417–423 ↗(On Diff #83101)

Ditto.

428–437 ↗(On Diff #83101)

Ditto.

This revision was automatically updated to reflect the committed changes.