Page MenuHomePhabricator

[LICM] Cap the clobbering calls in LICM.
ClosedPublic

Authored by asbirlea on Feb 8 2019, 11:47 AM.

Details

Summary

Unlimitted number of calls to getClobberingAccess can lead to high
compile times in pathological cases.
Switching EnableLicmCap flag from bool to int, and enabling to default 100.
(tested to be appropriate for current bechmarks)
We can revisit this value when enabling MemorySSA.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Feb 8 2019, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 11:47 AM
Herald added a subscriber: jlebar. · View Herald Transcript
This revision is now accepted and ready to land.Feb 8 2019, 12:51 PM
asbirlea updated this revision to Diff 186060.Feb 8 2019, 3:32 PM

I disregarded the possiblity of threading in the initial diff.
Updated to pass the counter as an argument.
Kindly re-review.

Thanks for this!

One currently-actionable nit from me

include/llvm/Transforms/Utils/LoopUtils.h
115 ↗(On Diff #186060)

Nit: I'm OK with this as is, but if we grow another MSSA-related optimization limit/counter, please wrap the MSSA-related bits into some sort of options struct (a bit in spirit like llvm::ObjectSizeOpts, but we'd also be tracking the mutable EnableLicmCapCounter/etc in it)

lib/Transforms/Scalar/LICM.cpp
119 ↗(On Diff #186060)

nit: enable sounds pretty bool-y for an int. Can we swap to licm-mssa-optimization-cap (or similar)?

asbirlea updated this revision to Diff 186323.Feb 11 2019, 1:32 PM
asbirlea marked 2 inline comments as done.

Address comment (renamed the flag, and the counter to match it).

asbirlea marked an inline comment as done.Feb 11 2019, 1:32 PM
asbirlea added inline comments.
include/llvm/Transforms/Utils/LoopUtils.h
115 ↗(On Diff #186060)

Noted, and I agree! I considered adding this in this patch, actually.
Additional refactoring may help somewhat, since the only user of sinkRegion and hoistRegion is LICM. We'd still needs these bits wrapped up for canSinkAndHoist which is used in LoopSink.
Leaving it as a separate cleanup for now.

george.burgess.iv marked an inline comment as done.

Deferring to Chandler's LGTM for the approach + first patch; changes since then LGTM.

Thanks again!

include/llvm/Transforms/Utils/LoopUtils.h
115 ↗(On Diff #186060)

SGTM

This revision was automatically updated to reflect the committed changes.