Page MenuHomePhabricator

[EarlyCSE & MSSA] Cap the clobbering calls in EarlyCSE.
ClosedPublic

Authored by asbirlea on Feb 14 2019, 12:50 PM.

Details

Summary

Unlimitted number of calls to getClobberingAccess can lead to high
compile times in pathological cases.
Limitting getClobberingAccess to a fairly high number. Can be adjusted
based on users/need.
Note: this is the only user of MemorySSA currently enabled by default.
The same handling exists in LICM (disabled atm). As MemorySSA gains more
users, this logic of capping will need to move inside MemorySSA.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Feb 14 2019, 12:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 12:50 PM

Very cool, thank you for coming up with a great way to improve this case.

Thanks for doing this! Just two nits for you :)

lib/Transforms/Scalar/EarlyCSE.cpp
672 ↗(On Diff #186899)

Please add a TODO with notes about moving this to MSSA and maybe the link to the motivating patch (https://reviews.llvm.org/D56720)

1206 ↗(On Diff #186899)

Should we be resetting ClobberCounter here, as well?

asbirlea marked an inline comment as done.Feb 15 2019, 1:37 PM
asbirlea added inline comments.
lib/Transforms/Scalar/EarlyCSE.cpp
1206 ↗(On Diff #186899)

AFAICT run() is only invoked once for an EarlyCSE instance and it's not recursive.

I'm also missing the purpose of CurrentGeneration = LiveOutGeneration; and
the above

// Save the current generation.
unsigned LiveOutGeneration = CurrentGeneration;

Could you please clarify?

george.burgess.iv added inline comments.
lib/Transforms/Scalar/EarlyCSE.cpp
1206 ↗(On Diff #186899)

AFAICT run() is only invoked once for an EarlyCSE instance and it's not recursive.

That's what I thought, too, but I saw this, so I assumed I was missing something.

Looks like that was added circa 2012; I assume this pass has changed quite a lot since then. I'm hoping davide (or one of the subscribers) may have context on EarlyCSE's usage patterns?

If no one is sure, I vote that we replace it with an early assert(!CurrentGeneration && "Create a new EarlyCSE instance to run it again") (which can optionally be removed if no one complains within a reasonable amount of time).

asbirlea updated this revision to Diff 187084.Feb 15 2019, 2:10 PM

Address comment (add TODO).

asbirlea marked an inline comment as done.Feb 15 2019, 2:12 PM
asbirlea added inline comments.
lib/Transforms/Scalar/EarlyCSE.cpp
1206 ↗(On Diff #186899)

I agree with cleaning up existing code, but I will argue against doing it in this patch. Happy to send a follow up one adding the assert.

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

Thanks again!

lib/Transforms/Scalar/EarlyCSE.cpp
1206 ↗(On Diff #186899)

IMO, if we're not asserting that this 'cleanup' code is useless as a part of this patch, we should be cleaning up the MSSA counter until those assertions do land.

Not a strong enough nit for me to block this patch on, since I agree that it's probably a nop in any case. Your call. :)

This revision is now accepted and ready to land.Feb 15 2019, 2:24 PM
asbirlea marked 4 inline comments as done.Feb 15 2019, 2:46 PM
asbirlea added inline comments.
lib/Transforms/Scalar/EarlyCSE.cpp
1206 ↗(On Diff #186899)

Tested with the assert, all seems fine (i.e. those assignments are nops).
I'll go ahead and send this out to unblock the folks hitting the pathological compile times.
Thanks a lot for the review!
(I'll send the clean-up patch shortly as well)

This revision was automatically updated to reflect the committed changes.