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

Mon Feb 10 07:52:41 PST 2020

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.

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

+  // Infer attributes on declarations, call sites, arguments, etc.
+  MPM.add(createAttributorLegacyPass());
----------------
fhahn wrote:
> 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
> 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?

I was hoping it would look at `noescape`, `readonly`, and similar things, e.g., to resolve the loads from private globals that we talked about the other day. I haven't verified that.

We can move it, given that we do not have data either way "it doesn't matter". I hoped the above would hold true and that would be a good argument but at the end of the day we can just collect statistics in both orders and compare.

> There's a single place to find the options to enable/disable various passes

That is already not true, as I noted before, a few examples:
  llvm/lib/Analysis/AliasAnalysis.cpp:61
  llvm/lib/Transforms/Scalar/LoopDistribute.cpp:118
  llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:67
  llvm/lib/CodeGen/TypePromotion.cpp:50

Also, there will be at least two places (both PMs).

> 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)

That is a fair point I guess.

> All attributor tests have to pass -disable-attributor=false, right?

Yes, though that doesn't seem to be much of a problem.

> 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

I don't think it is helping but I'll make a patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70767