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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

some comments on the summary

- we should clarify that this is under a flag and is not currently changing any defaults. [2] says that, but we should make sure it's very clear somewhere besides a footer
- the simplification pipeline is known to not be idempotent (and it would be very hard to make it so without majorly regressing compile times), so `should not have a code quality effect` is not true at all. but we should note that in a perfect world this wouldn't change code quality if the function simplification were perfect
- `the function passes will be re-run` -> `the function simplification pipeline will be re-run` is a bit clearer, same with `This patch allows the function passes be skipped if they were run once and a function was not modified since.`



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


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:594
+      CurrentC = &updateCGAndAnalysisManagerForPass(CG, *CurrentC, *N, AM, UR,
+                                                    FAM, true, false);
       assert(CG.lookupSCC(*N) == CurrentC &&
----------------
nit: `/*FunctionPass*/ true, /*InvalidateSA*/ false`


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:906
+
+  if (InvalidateSA)
+    FAM.invalidate<FunctionStatusAnalysis>(N.getFunction());
----------------
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?


================
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:
> also check if we rerun SROA on f3?
sorry, I also meant to check that we don't rerun it in `NOREPS`


================
Comment at: llvm/test/Other/new-pass-manager-cgscc-fct-proxy.ll:39
+; CHECK:          Starting CGSCC pass manager run.
+; CHCEK-NEXT:     Running pass: InlinerPass on (f3)
----------------
aeubanks wrote:
> typo
still typo `CHCEK-NEXT`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98103