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

[Johannes Doerfert via Phabricator via llvm-commits]

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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70767