[PATCH] D70767: [Attributor] Add an Attributor CG-SCC pass

Mon Feb 10 03:14:43 PST 2020

fhahn added a subscriber: fedor.sergeev.
fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:525

+  // Infer attributes on declarations, call sites, arguments, etc.
+  MPM.add(createAttributorLegacyPass());
----------------
jdoerfert wrote:
> fhahn wrote:
> > jdoerfert wrote:
> > > uenoku wrote:
> > > > Do you have any reason why you move Attributor here?
> > > The module pass should run early to improve the passes before the inliner loop, e.g., createCalledValuePropagationPass below.
> > Have you considered running it directly after IPSCCP? Then it would benefit from the existing constant propagation.
> > 
> > Also, running it unconditionally and then disabling it in the pass seems a bit odd (I think it was also discussed in another review which got abandoned IIRC). Is there a reason to not add it conditionally like we do with lots of other passes?
> > Have you considered running it directly after IPSCCP? Then it would benefit from the existing constant propagation.
> 
> Its the classical conundrum. The `nocapture` and memory annotations can help IPSCCP and propagated constants can help the Attributor. I don't have numbers on both combinations so I picked one for now. My reasoning was as follows: The Attributor can propagate constants interprocedurally, so easy ones it will do on its own and it will also improve the result based on the propagation (the "conditional" part of SCCP). The hard ones are left for IPSCCP which has better changes given all the annotations. TBH, IPSCCP should run as part of the Attributor, not as a rewrite per se but with a thin layer to integrate it. Until then, I'm fine with any order as long as the Attributor Module pass runs early.
> 
> > Also, running it unconditionally and then disabling it in the pass seems a bit odd (I think it was also discussed in another review which got abandoned IIRC). Is there a reason to not add it conditionally like we do with lots of other passes?
> 
> It was, I replied there as well [0]. I can move the flag that disables the Attributor into two files and replicate the conditional that checks it in total 5 times but I fail to see the benefit (both short and long term). In the short term it will almost not make a difference to the pass pipeline, sure the pass is not scheduled but as far as I can tell it does never invalidate anything so calling `runXXX` and immediately should be hard to find in a profile. In the hopefully short term the flag default will flip making it an awkward flag to have in the pass pipeline. If I remember correctly we have various passes that can be disabled via a flag and most of them have the flag in their respective pass file. This is no different except that we haven't flipped the default yet. Long story short, if there is a benefit or if you feel strongly about this I can move the conditional, otherwise I would just continue working on enabling it by default and once it is it's not awkward anymore.
> 
> 
> [0] https://reviews.llvm.org/D69596#1842179
> 
> Its the classical conundrum. The nocapture and memory annotations can help IPSCCP and propagated constants can help the Attributor. I don't have numbers on both combinations so I picked one for now. My reasoning was as follows: The Attributor can propagate constants interprocedurally, so easy ones it will do on its own and it will also improve the result based on the propagation (the "conditional" part of SCCP). The hard ones are left for IPSCCP which has better changes given all the annotations. TBH, IPSCCP should run as part of the Attributor, not as a rewrite per se but with a thin layer to integrate it. Until then, I'm fine with any order as long as the Attributor Module pass runs early.

Does SCCP use any of the memory annotations? I had a quick look and I could not find any place it looked at attributes. I might be missing something, but if it doesn't use any attributes, it should be fairly safe to move it?

> It was, I replied there as well [0]. I can move the flag that disables the Attributor into two files and replicate the conditional that checks it in total 5 times but I fail to see the benefit (both short and long term). In the short term it will almost not make a difference to the pass pipeline, sure the pass is not scheduled but as far as I can tell it does never invalidate anything so calling runXXX and immediately should be hard to find in a profile. In the hopefully short term the flag default will flip making it an awkward flag to have in the pass pipeline. If I remember correctly we have various passes that can be disabled via a flag and most of them have the flag in their respective pass file. This is no different except that we haven't flipped the default yet. Long story short, if there is a benefit or if you feel strongly about this I can move the conditional, otherwise I would just continue working on enabling it by default and once it is it's not awkward anymore.

Adding it conditionally in PassManagerBuilder has a few slight advantages IMO:
* There's a single place to find the options to enable/disable various passes
* Looking at the pass pipeline, you can be reasonably sure to know which passes are actually run. IMO having passes in the pipeline but not being run seems quite confusing (i.e. they will show up in the list of passes run, print-*-all will show them)
* All attributor tests have to pass -disable-attributor=false, right?

I think as long as it is disabled by default adding it conditionally provides better ergonomics. I think @fedor.sergeev had similar comments at D69596

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70767