[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Leonard Chan via Phabricator via llvm-commits]

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