xur added a comment. In D63155#1563266 <https://reviews.llvm.org/D63155#1563266>, @chandlerc wrote:
> In D63155#1563240 <https://reviews.llvm.org/D63155#1563240>, @leonardchan > wrote: > > > In D63155#1563229 <https://reviews.llvm.org/D63155#1563229>, @xur wrote: > > > > > This patch does not make sense to me. > > > > > > The reason of failing with -fexperimental-new-pass-manager is because we > > > don't do PGO instrumentation at -O0. This is due to the fact that PGO > > > instrumentation/use passes are under > > > PassBuilder::buildPerModuleDefaultPipeline. > > > > > > This patch add a pass > > > > > > MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile)); > > > > > > which only gives you the wrong signal of instrumentation is done. > > > > > > I wrote pass PGOInstrumentationGenCreateVar only for some trick > > > interactions for thinlto under ldd for CSPGO. > > > Regular FDO should not use it. > > > > > > The right fix is to enable PGO instrumentation and use in pass builder > > > for O0. > > > > > > I would like to request to revert this patch. > > > > > > As an alternative, could I instead request that we remove the BackendUtil > > changes and just mark the runs in gcc-flag-compatibility.c with > > `-fno-experimental-new-pass-manager`. This way we could clarify that under > > the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as > > is. > > > No, I think we should be doing PGO at O0 in the new PM if we do so in the old > PM. > > I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct > to fix this test case. That seems plausible to me at least, and I think > reverting and figuring out what the right way to do it is a fine approach. Just check the IR, we still not doing instrumentation at O0. This patch just mask the error. > I just think we also need to understand why *no other test failed*, and why > the only test we have for doing PGO at O0 is this one and it could be "fixed" > but such a deeply unrelated change.... We have special code to do PGO at O0 in old pass manager. This is not done in the new pass manager. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63155/new/ https://reviews.llvm.org/D63155 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
