aeubanks marked an inline comment as done. aeubanks added inline comments.
================ 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")); ---------------- tejohnson wrote: > 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? added a new test in the vein of new-pm-defaults.ll, could you see if the printed passes make sense? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91585/new/ https://reviews.llvm.org/D91585 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
