chandlerc added a comment. 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. 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.... 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
