[PATCH] D145210: [Pipeline] Adjust PostOrderFunctionAttrs placement in simplification pipeline

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:838
+  // functions.
+  MainCGPipeline.addPass(PostOrderFunctionAttrsPass(/*SkipNonRecursive*/ true));
 
----------------
cfang wrote:
> cfang wrote:
> > aeubanks wrote:
> > > cfang wrote:
> > > > aeubanks wrote:
> > > > > cfang wrote:
> > > > > > We found that "nofree" attribute will greatly affect the earlyCSE pass in function simplification. In our case, missing nofree essentially disabled earlyCSE which resulted in a huge performance regression (register pressure).   Other optimizations like GVN in function simplification may also be affected by function attributes.
> > > > > > 
> > > > > > So is it possible that we just run this pass twice?   Thanks.
> > > > > Can you give an example where EarlyCSE depends on function attributes? That doesn't really make sense to me, function attributes should really be used by callers of the function, not the function itself.
> > > > EarlyCSE can make use of the attributes of the functions it calls. 
> > > > The pass manager should be running passes such that the attributes
> > > > of callees are known before running optimizations on the callers
> > > yes the pass manager already takes care of visiting callees before callers with the CGSCCPassManager. the only case where we need to run function-attrs before the function simplification pipeline is when you have recursive functions, which this patch handles.
> > > 
> > > so I'm still not understanding where you'd be seeing a regression
> > MainCGPipeline.addPass(PostOrderFunctionAttrsPass(/*SkipNonRecursive*/ false));
> > 
> > I switch SkipNonRecursive between false and true , and compare IR around the following earlyCSE pass in function simplification pipeline: 
> > 
> >   FPM.addPass(EarlyCSEPass(true /* Enable mem-ssa. */));
> > 
> > 1. Before this pass,  the only difference is that when SkipNonRecursive == false; there is a nofree function attribute, but I am not clear about the difference in the callee. Some of them seem losing the "convergent" attribute
> > 
> > 2. After this earlyCSE pass, we see a huge IR difference, which I don't know the reason yet. But the difference is due to  SkipNonRecursive flip. 
> > 
> > So something goes not what we think it should be. 
> >   
> addArgumentAttrs(Nodes.SCCNodes, Changed);
> 
> Looks to me this above function was not called to deduce function argument attributes. 
> For our case, missing WriteOnly/ReadOnly attributes affects memorySSA for earlyCSE, and we have observed may laods are not CSEd.
Please provide a PhaseOrdering test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145210