chandlerc added a comment. FWIW, I think we can wait for a bug to be filed or report come in with some functional test case before we change any behavior here.
I'm just suggesting how to make the test better below. ================ Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668 + // We must also remove exception handling before attaching sample profile + // data. + MPM.addPass( + createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass())); ---------------- leonardchan wrote: > tejohnson wrote: > > chandlerc wrote: > > > Does the this need to go before the EarlyFPM as well as before the sample > > > profile loader? > > > > > > Also, this is ... a pretty expensive thing to do... Do we really need > > > this? We've been using ThinLTO and sample PGO with the new PM for a long > > > time and haven't seen any real issue. I think we can probably just leave > > > this difference between the two pass managers and remove the test for > > > this pass running rather than adding it. > > > > > > CC-ing some others just to double check, but I'm not aware of any issues > > > we've seen with PGO that were because of this discrepancy. > > We don't tend to use exception handling so might not be hitting the issue > > described in the headline. > > > > If this is fixing an issue with SamplePGO and EH cleanup interactions, can > > you add a test that tests for that (i.e. is there a code optimization issue > > currently?). > I'm not entirely familiar with the expected codegen for SamplePGO or PruneEH > and don't really know if not adding these 2 passes breaks anything. > > Under the legacy PM, exception handling was removed via `PruneEH` before > creating the sample profile loader, so this change was meant to match the > behavior since `function-attrs` and `function(simplify-cfg)` (the new PM > equivalent for `-prune-eh`) did not initially run before > `SampleProfileLoaderPass` was added to the pipeline. > > The original patch that added `CodeGen/pgo-sample.c` (D21197) doesn't seem to > have any codegen tests either and only checks that PruneEH runs before sample > profile loader creation, so I'm also unsure of what code optimizations are > meant to happen. Going back to this original issue, I think I understand more what is going on... Specifically, I can imagine that having `invoke` still in place caused some issues. So I would just keep the testing that (in the new PM) SimplifyCFG is run before the profile loading pass, and ignore function attrs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63626/new/ https://reviews.llvm.org/D63626 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
