[PATCH] D98103: [NPM] Do not run function simplification pipeline unnecessarily

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 4 inline comments as done.
mtrofin added a comment.

In D98103#2684402 <https://reviews.llvm.org/D98103#2684402>, @asbirlea wrote:

> I agree with @aeubanks on removing the NFC from the title. This is changing the functionality, even if it's under a disabled flag.
>
> For additional context, with the flag enabled we're seeing performance regressions which indicate optimizations are being missed and picked up by the subsequent runs of the optimization pass.
> The purpose of this initial patch is to stage the transition by first introducing the flag, then focus on understanding where the missed optimizations occur so we can fix (for lack of a better word) the NPM pass pipeline.
> When the flag is eventually flipped, it's possible the compile-time benefits also disappear due to the introduction of additional optimization passes, but the current pathological cases of high compile times due to repeating (truly unnecessary) optimizations, should remain resolved.





================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:562-563
+    // here and we don't need to rerun the passes managed by this adaptor.
+    // The assumption here is that CGSCC passes are well-behaved, in that, upon
+    // changing a function, they call updateCGAndAnalysisManagerForCGSCCPass.
+    if (FAM.getCachedResult<FunctionStatusAnalysis>(F))
----------------
aeubanks wrote:
> I don't think this is necessarily true. ArgumentPromotion is a CGSCC pass that changes functions but doesn't change the call graph so it doesn't need to call `updateCGAndAnalysisManagerForCGSCCPass()`. It handles invalidating analyses itself.
> Same with PostOrderFunctionAttrsPass, but it just invalidates everything if anything has changed.
ArgumentPromotion outright deletes the function and clears the AM. I made the comment more general.


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:906
+
+  if (InvalidateSA)
+    FAM.invalidate<FunctionStatusAnalysis>(N.getFunction());
----------------
aeubanks wrote:
> mtrofin wrote:
> > aeubanks wrote:
> > > can this be `!FunctionPass`?
> > then we wouldn't invalidate when updateCGAndAnalysisManagerForFunctionPass is called. CoroSplit calls it, for instance. 
> I feel like I'm missing something obvious, why is this invalidate here necessary? Can we just see what the passes invalidate?
When passes call into here, I we can assume FunctionStatusAnalysis can be invalidated because something changed.

But this is also entered from line 596, which happens after the function simplification pipeline finishes. The last thing that the pipeline does is it caches a FunctionStatusAnalysis::Result. Well, so next thing that would happen is we'd yank it here - unless we indicate this is the one case we don't want that to happen.


================
Comment at: llvm/test/Other/new-pass-manager-cgscc-fct-proxy.ll:38
+
+; CHECK:          Starting CGSCC pass manager run.
+; CHCEK-NEXT:     Running pass: InlinerPass on (f3)
----------------
aeubanks wrote:
> aeubanks wrote:
> > also check if we rerun SROA on f3?
> sorry, I also meant to check that we don't rerun it in `NOREPS`
oh - we still run it there. I think there's one more inlining happening in f3.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98103