[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:2365
+        Matches[1] != "lto") {
+      MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default"));
       return Error::success();
----------------
I think it would be clearer to check against "thinlto-pre-link" and "lto-pre-link" here (and more robust in case any other types added in the future).


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:2365
+      // Don't do anything for (thin)lto backend compiles at O0.
+      if (Matches[1] != "thinlto" && Matches[1] != "lto")
+        MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default"));
----------------
aeubanks wrote:
> aeubanks wrote:
> > tejohnson wrote:
> > > This seems to change behavior. For one, previously we were only suppressing adding the PGO Instr passes for ThinLTO. Now this will suppress adding the coroutines passes and whatever else runRegisteredEPCallbacks was doing. Also, it's now doing the same for regular LTO but it didn't seem to do any special handling in that case before. Are these changes intended?
> > The callbacks are generally for two purposes. One is to lower certain constructs, which only needs to be done once. It should already have been done in the pre-link step. The other is for optimization purposes at specific points in the pipeline. Since this is -O0, that doesn't matter.
> > 
> > So I think this makes sense.
> Actually, I just noticed that build(Thin)LTODefaultPipeline does handle O0. The default and LTO pre-link ones assert when passed O0 so I assumed that was the case for the LTO pipelines, but that's not the case. I'll change that.
Ok, just to confirm, it seems like this code was incorrectly adding additional passes before for thinlto<O0> (coroutines, runRegisteredEPCallbacks) and for lto<O0> (pgo, coroutines, runRegisteredEPCallbacks) - is that correct?

I would have expected some changes in the new PM pipeline tests, but it looks like there is a lack of testing of these two. I don't see any pipeline tests for lto<O0>, and only one for thinlto<O0> - and that only checks the PGO passes (llvm/test/Other/new-pm-pgo-O0.ll). Can you add lto<O0> to that test? Can you also beef it up to add some of the other passes that (I think) we were previously adding below here for thinlto and lto that we won't anymore?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91585