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

[leonardchan]

Summary

This patch ensures that SimplifyCFGPass comes before SampleProfileLoaderPass on PGO runs in the new PM and fixes clang/test/CodeGen/pgo-sample.c.

[chandlerc]

See inline comment, but I think we should just drop the testing of the function attribute bit here rather than adjusting the pipeline.

llvm/lib/Passes/PassBuilder.cpp

665–668 ↗

(On Diff #205904)

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.

[tejohnson]

Adding Wei who works on SamplePGO on our end currently.

llvm/lib/Passes/PassBuilder.cpp

665–668 ↗

(On Diff #205904)

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?).

[davidxl]

Is there a bug reference somewhere?

[leonardchan]

In D63626#1553054, @chandlerc wrote:

See inline comment, but I think we should just drop the testing of the function attribute bit here rather than adjusting the pipeline.

Ok. Removed the pipeline change for now.

In D63626#1553646, @davidxl wrote:

Is there a bug reference somewhere?

No bug has been filed for this, but I'm not sure if this is a bug if there aren't really any issues with PGO under the new PM.

llvm/lib/Passes/PassBuilder.cpp

665–668 ↗

(On Diff #205904)

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.

[chandlerc]

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.

llvm/lib/Passes/PassBuilder.cpp

665–668 ↗

(On Diff #205904)

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.

[leonardchan]

llvm/lib/Passes/PassBuilder.cpp

665–668 ↗

(On Diff #205904)

Done.

[chandlerc]

LGTM

cfe/trunk/test/CodeGen/pgo-sample.c

 // Test if PGO sample use passes are invoked.
 //
 // Ensure Pass PGOInstrumentationGenPass is invoked.
-// RUN: %clang_cc1 -O2 -fprofile-sample-use=%S/Inputs/pgo-sample.prof %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s
-// CHECK: Remove unused exception handling info
-// CHECK: Sample profile pass
+// RUN: %clang_cc1 -O2 -fno-experimental-new-pass-manager -fprofile-sample-use=%S/Inputs/pgo-sample.prof %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=LEGACY
+// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager -fprofile-sample-use=%S/Inputs/pgo-sample.prof %s -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=NEWPM
+
+// LEGACY: Remove unused exception handling info
+// LEGACY: Sample profile pass
+
+// NEWPM: SimplifyCFGPass
+// NEWPM: SampleProfileLoaderPass
+
+int func(int a) { return a; }