[PATCH] D58248: [EarlyCSE & MSSA] Cap the clobbering calls in EarlyCSE.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 4 inline comments as done.
asbirlea added inline comments.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:1206
   // Reset the current generation.
   CurrentGeneration = LiveOutGeneration;
 
----------------
george.burgess.iv wrote:
> asbirlea wrote:
> > george.burgess.iv wrote:
> > > asbirlea wrote:
> > > > george.burgess.iv wrote:
> > > > > Should we be resetting `ClobberCounter` here, as well?
> > > > 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?
> > > > 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). 
> > 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.
> 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. :)
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)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58248/new/

https://reviews.llvm.org/D58248