leonardchan added inline comments.
================ Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668 + // We must also remove exception handling before attaching sample profile + // data. + MPM.addPass( + createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass())); ---------------- chandlerc wrote: > 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. > Done. 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
