Move SampleProfileLoader pass before all simplification passes.

[danielcdh]

Summary

This makes the new PM not matching the legacy PM. But testing shows that it improves performance because simplification passes especially simplify CFG can affect the accuracy of the debug info.

[chandlerc]

lib/Passes/PassBuilder.cpp

536–537

Since you're now adding the only cleanup you need before loading the sample profile, can't you lift this back into the caller to simplify things? (Or does that not simplify things any more?)

I'm happy for that to be a separate patch if you want to see what the performance numbers end up looking like first.

537–538

This comment needs updating

[danielcdh]

lib/Passes/PassBuilder.cpp

536–537

That's a good point. Moved the InstCombine pass to the caller to make the logic clearer.

[danielcdh]

update

[davidxl]

Is there a functional test case (i.e., better matching with this change)?

While it does not matter in the longer run, it is probably better to not diverge the sample PGO behavior of the new PM with legacy PM too much, so perhaps make the same change to legacy PM?

[danielcdh]

In D36655#840188, @davidxl wrote:

Is there a functional test case (i.e., better matching with this change)?

Looks like function tests like this is not welcomed in llvm/clang as unittests are supposed to test a single component instead of multiple components. Maybe we can make it into part of the test that we run outside of llvm/clang to validate performance?

While it does not matter in the longer run, it is probably better to not diverge the sample PGO behavior of the new PM with legacy PM too much, so perhaps make the same change to legacy PM?

I just looked at the possibility of changing this in legacy PM, it will be non-trivial to do it because SampleProfileLoader pass is a module pass, which needs to run after AddDiscriminator pass, which is a function pass that runs in populateFunctionPassManager. And the simplification passes also runs in that function. We will need to break populateFunctionPassManager into 2 parts to invoke AddDiscriminator pass first, and then run SampleProfileLoader module pass, and then populateFunctionPassManager. Needs to change a lot across clang and llvm PassManagerBuilder.

[chandlerc]

The code change LGTM.

To David's point, we do have tests that span basic functionality, so I don't really see a problem with testing this?

The thing we typically try to avoid are tests that span a *frontend* and LLVM proper, but it seems like this could be expressed without that by just having some really minimal IR with debug info that simplify-cfg will destroy, and loading a sample profile into that IR....

That said, if David is OK with it, I'd be happy with the test being in a follow-up commit if it will take a while to craft.

[davidxl]

A follow up patch for a test is fine.

lgtm

lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
                                                ThinLTOPhase Phase,
                                                bool DebugLogging) {
   ModulePassManager MPM(DebugLogging);
 
   // Do basic inference of function attributes from known properties of system
   // libraries and other oracles.
   MPM.addPass(InferFunctionAttrsPass());
 
-  // Create an early function pass manager to cleanup the output of the
-  // frontend.
-  FunctionPassManager EarlyFPM(DebugLogging);
-  EarlyFPM.addPass(SimplifyCFGPass());
-  EarlyFPM.addPass(SROA());
-  EarlyFPM.addPass(EarlyCSEPass());
-  EarlyFPM.addPass(LowerExpectIntrinsicPass());
-  // In SamplePGO ThinLTO backend, we need instcombine before profile annotation
-  // to convert bitcast to direct calls so that they can be inlined during the
-  // profile annotation prepration step.
-  // More details about SamplePGO design can be found in:
-  // https://research.google.com/pubs/pub45290.html
-  // FIXME: revisit how SampleProfileLoad/Inliner/ICP is structured.
-  if (PGOOpt && !PGOOpt->SampleProfileFile.empty() &&
-      Phase == ThinLTOPhase::PostLink)
-    EarlyFPM.addPass(InstCombinePass());
-  MPM.addPass(createModuleToFunctionPassAdaptor(std::move(EarlyFPM)));
-
   if (PGOOpt && !PGOOpt->SampleProfileFile.empty()) {
-    // Annotate sample profile right after early FPM to ensure freshness of
+    // Annotate sample profile before early FPM to ensure freshness of
     // the debug info.
     MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
     // Do not invoke ICP in the ThinLTOPrelink phase as it makes it hard
     // for the profile annotation to be accurate in the ThinLTO backend.
     if (Phase != ThinLTOPhase::PreLink)
       // We perform early indirect call promotion here, before globalopt.
       // This is important for the ThinLTO backend phase because otherwise
       // imported available_externally functions look unreferenced and are
       // removed.
       MPM.addPass(PGOIndirectCallPromotion(Phase == ThinLTOPhase::PostLink,
                                            true));
   }
 
+  // Create an early function pass manager to cleanup the output of the
+  // frontend.
+  FunctionPassManager EarlyFPM(DebugLogging);
+  EarlyFPM.addPass(SimplifyCFGPass());
+  EarlyFPM.addPass(SROA());
+  EarlyFPM.addPass(EarlyCSEPass());
+  EarlyFPM.addPass(LowerExpectIntrinsicPass());
+  MPM.addPass(createModuleToFunctionPassAdaptor(std::move(EarlyFPM)));
+
   // Interprocedural constant propagation now that basic cleanup has occured
   // and prior to optimizing globals.
   // FIXME: This position in the pipeline hasn't been carefully considered in
   // years, it should be re-analyzed.
   MPM.addPass(IPSCCPPass());
 
   // Optimize globals to try and fold them into constants.
   MPM.addPass(GlobalOptPass());
PassBuilder::buildThinLTODefaultPipeline(OptimizationLevel Level,
   // here, before globalopt. Otherwise imported available_externally functions
   // look unreferenced and are removed.
   // FIXME: move this into buildModuleSimplificationPipeline to merge the logic
   //        with SamplePGO.
   if (!PGOOpt || PGOOpt->SampleProfileFile.empty())
     MPM.addPass(PGOIndirectCallPromotion(true /* InLTO */,
                                          false /* SamplePGO */));
 
+  // In SamplePGO ThinLTO backend, we need instcombine before profile
+  // annotation to convert bitcast to direct calls so that they can be
+  // inlined during the profile annotation prepration step.
+  // More details about SamplePGO design can be found in:
+  // https://research.google.com/pubs/pub45290.html
+  // FIXME: revisit how SampleProfileLoad/Inliner/ICP is structured.
+  if (PGOOpt && !PGOOpt->SampleProfileFile.empty())
+    MPM.addPass(createModuleToFunctionPassAdaptor(InstCombinePass()));
+
   // Add the core simplification pipeline.
   MPM.addPass(buildModuleSimplificationPipeline(Level, ThinLTOPhase::PostLink,
                                                 DebugLogging));
 
   // Now add the optimization pipeline.
   MPM.addPass(buildModuleOptimizationPipeline(Level, DebugLogging));
 
   return MPM;

test/Other/new-pm-pgo.ll

 ; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-kind=new-pm-pgo-instr-gen-pipeline -profile-file='temp' %s 2>&1 |FileCheck %s --check-prefixes=GEN
 ; RUN: llvm-profdata merge %S/Inputs/new-pm-pgo.proftext -o %t.profdata
 ; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-kind=new-pm-pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE
 ; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-kind=new-pm-pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-pgo.prof' %s 2>&1 \
 ; RUN:     |FileCheck %s --check-prefixes=SAMPLE_USE,SAMPLE_USE_O
 ; RUN: opt -debug-pass-manager -passes='thinlto-pre-link<O2>' -pgo-kind=new-pm-pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-pgo.prof' %s 2>&1 \
 ; RUN:     |FileCheck %s --check-prefixes=SAMPLE_USE,SAMPLE_USE_PRE_LINK
 ; RUN: opt -debug-pass-manager -passes='thinlto<O2>' -pgo-kind=new-pm-pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-pgo.prof' %s 2>&1 \
 ; RUN:     |FileCheck %s --check-prefixes=SAMPLE_USE,SAMPLE_USE_POST_LINK
 ; RUN: opt -debug-pass-manager -passes='default<O2>' -new-pm-debug-info-for-profiling %s 2>&1 |FileCheck %s --check-prefixes=SAMPLE_GEN
 ;
 ; GEN: Running pass: PGOInstrumentationGen
 ; USE: Running pass: PGOInstrumentationUse
 ; SAMPLE_USE_O: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
 ; SAMPLE_USE_PRE_LINK: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
-; SAMPLE_USE: Running pass: SimplifyCFGPass
-; SAMPLE_USE: Running pass: SROA
-; SAMPLE_USE: Running pass: EarlyCSEPass
-; SAMPLE_USE: Running pass: LowerExpectIntrinsicPass
-; SAMPLE_USE_POST_LINK: Running pass: InstCombinePass
+; SAMPLE_USE-NOT: Running pass: SimplifyCFGPass
+; SAMPLE_USE-NOT: Running pass: SROA
+; SAMPLE_USE-NOT: Running pass: EarlyCSEPass
+; SAMPLE_USE-NOT: Running pass: LowerExpectIntrinsicPass
+; SAMPLE_USE_POST_LINK: Running pass: ModuleToFunctionPassAdaptor<{{.*}}InstCombinePass{{.*}}>
 ; SAMPLE_USE: Running pass: SampleProfileLoaderPass
 ; SAMPLE_USE_O: Running pass: PGOIndirectCallPromotion
 ; SAMPLE_USE_POST_LINK-NOT: Running pass: GlobalOptPass
 ; SAMPLE_USE_POST_LINK: Running pass: PGOIndirectCallPromotion
 ; SAMPLE_GEN: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
 
 define void @foo() {
   ret void
 }