This is an archive of the discontinued LLVM Phabricator instance.

[LICM/MSSA] Add promotion to scalars by building an AliasSetTracker with MemorySSA.
ClosedPublic

Authored by asbirlea on Jan 11 2019, 5:07 PM.

Details

Summary

Experimentally we found that promotion to scalars carries less benefits
than sinking and hoisting in LICM. When using MemorySSA, we build an
AliasSetTracker on demand in order to reuse the current infrastructure.
We only build it if less than AccessCapForMSSAPromotion exist in the
loop, a cap that is by default set to 250. This value ensures there are
no runtime regressions, and there are small compile time gains for
pathological cases. A much lower value (20) was found to yield a single
regression in the llvm-test-suite and much higher benefits for compile
times. Conservatively we set the current cap to a high value, but we will
explore lowering it when MemorySSA is enabled by default.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Jan 11 2019, 5:07 PM
asbirlea updated this revision to Diff 183196.Jan 23 2019, 3:06 PM

Minor update and ping.

sanjoy added inline comments.Jan 25 2019, 2:21 PM
lib/Analysis/AliasSetTracker.cpp
523 ↗(On Diff #183196)

Maybe call this addAllInstructionsInLoopUsingMSSA? A bit long, but more descriptive I think.

lib/Transforms/Scalar/LICM.cpp
125 ↗(On Diff #183196)

Let's make this a bit more descriptive -- how about "When MSSA in LICM is disabled, then this has no effect. When MSSA in LICM is enabled then ...".

2003 ↗(On Diff #183196)

CurAST is never null right? Can we make it a reference (or add an assert in LoopPromoter) and avoid checking it for null in LoopPromoter::replaceLoadWithValue etc.?

asbirlea updated this revision to Diff 183636.Jan 25 2019, 3:13 PM
asbirlea marked 3 inline comments as done.

Thank you for the comments! Addressed.

chandlerc accepted this revision.Jan 25 2019, 7:49 PM

Overall, really nice patch. A bunch of super nit-picky comments below, not much more to add beyond Sanjoy's review honestly. =D With these comments addressed, LGTM, but maybe double check if Sanjoy is happy w/ the update before submitting.

include/llvm/Transforms/Utils/LoopUtils.h
152–156 ↗(On Diff #183636)

Does clang-format only do this after your change? If not, I suspect either a weird configuration issue in your clang-format or it'd be nice to do the clang-format change first to make the diff more obvious here.

lib/Analysis/AliasSetTracker.cpp
321–323 ↗(On Diff #183636)

nit: I would just move these comments to not be trailing comments, use braces, and land that as a NFC commit rather than in this patch.

526–528 ↗(On Diff #183636)

Minor preference for Accesses and Access, makes it easier for me to read.

lib/Transforms/Scalar/LICM.cpp
120 ↗(On Diff #183636)

nit: Add vertical space above.

316 ↗(On Diff #183636)

Same naming comment as above.

2088 ↗(On Diff #183636)

Good place to use auto -- the RHS spells out the type.

This revision is now accepted and ready to land.Jan 25 2019, 7:49 PM
asbirlea marked 7 inline comments as done.Jan 28 2019, 11:59 AM
asbirlea added inline comments.
include/llvm/Transforms/Utils/LoopUtils.h
152–156 ↗(On Diff #183636)

AFAICT it's because of adding the additional MemorySSAUpdater * argument.

asbirlea updated this revision to Diff 183938.Jan 28 2019, 11:59 AM
asbirlea marked an inline comment as done.

Address comments.

sanjoy accepted this revision.Feb 6 2019, 11:58 AM

lgtm

Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 11:58 AM
This revision was automatically updated to reflect the committed changes.