Move the SampleProfileLoader right after EarlyFPM.

[danielcdh]

Summary

SampleProfileLoader pass do need to happen after some early cleanup passes so that inlining can happen correctly inside the SampleProfileLoader pass.

[tejohnson]

lib/Passes/PassBuilder.cpp

548

What is the downside of always doing InstCombine here?

555

Also needs comment about why before globalopt.

858–862

Can we simply invoke PGO ICP always from within buildModuleSimplificationPipeline if we're not in the ThinLTO PreLink phase? I.e. regardless of Sample or Instrumented PGO? It seems cleaner to do it in the same place if possible.

[danielcdh]

lib/Passes/PassBuilder.cpp

548

Increase compile time?

Additionally, I'd like to make this patch NFC for other compilation models.

555

comments added, also added test to cover that.

858–862

Yes, but I'd like to make this patch NFC for other modes such as instrumentation based PGO. Added FIXME

[danielcdh]

update

[tejohnson]

lib/Passes/PassBuilder.cpp

561

PGO ICP is called here for both the ThinLTO backend and the non-ThinLTO sample PGO case, right? If so, perhaps the comment should be modified slightly to "We perform early indirect call promotion here, before globalopt. This is important for the ThinLTO backend phase because otherwise imported..."

test/Other/new-pm-thinlto-defaults.ll

56

Where did this one go?

[danielcdh]

test/Other/new-pm-thinlto-defaults.ll

56

This change is irrelevant. I simply did it because I'm at it.

I changed to remove PGOIndirectCallPromotion from default ThinLTO pipeline if PGO is not enabled.

[danielcdh]

update comment

[tejohnson]

LGTM

[chandlerc]

lib/Passes/PassBuilder.cpp

555–556

Above, you say that this is because SampleProfileLoaderPass needs to be here because it inlines calls. Here you talk about debug info freshness... Which is it?

The debug info freshness makes plenty of sense to me. If SampleProfileLoaderPass is *doing inlining*, I find that deeply surprising and confusing. I'm not suggesting that we need to change the design of this right now, but I think the comment needs to be *really* clear because lots of other people will be surprised by this and I'd suggest a FIXME to revisit how this is structured. (Regardless of whether inlining makes sense here, I think it should *definitely* be separated from what appears to be a pass that merely annotates the IR from a profile.)

[davidxl]

lib/Passes/PassBuilder.cpp

555–556

Should probably add a reference to the autofdo CGO 2016 paper which has detailed description of how autofdo's context senstive profile data is organized and the profile driven inline and branch profile annotation design.

[danielcdh]

update

[danielcdh]

lib/Passes/PassBuilder.cpp

555–556

Yes, we do invoke InlineFunction directly in SampleProfileLoader. There is a great many details about how we end up with this design in out cgo2016 paper. And I could explain to you in person if you like.

The fundamental requirement is that we need to make the IR similar with the binary that is used for profiling, in order to annotate sample profile accurately.

To get to that goal, we have choose to first inline inside the SampleProfileLoader before annotation. During the inlining we need to make the inline decision by interacting with SampleProfile interface. By doing this inside the SampleProfileLoader, we lock the use of SampleProfile interface within this file, instead of making it escape into the inliner.

Alternatively, we could have a separate inliner pass to do this job. But that inliner pass need to read the SampleProfile too. So if we do that, we need to read the sample profile twice.

I'm open to discuss about other design alternatives too.

[danielcdh]

add cgo2016 paper reference

[danielcdh]

Thanks for the reviews.

Chandler, please let me know if you have more comments about this patch. If possible, I'd like to have committed before today's integration.

Thanks,
Dehao

[danielcdh]

ping...

I think Chandler's concerns for this patch has been addressed. We can continue discussion on the AutoFDO design in a separate thread. Will submit the patch by EOD if no more concerns have been raised.

[chandlerc]

In D36333#833963, @danielcdh wrote:

ping...

I think Chandler's concerns for this patch has been addressed. We can continue discussion on the AutoFDO design in a separate thread. Will submit the patch by EOD if no more concerns have been raised.

Sorry for delay,

Yes, all I was looking for was a comment so that a reader would better understand what the sample profile loading is doing (IE, transforming the IR). Not trying to suggest that we can or should change behavior here, just getting it more clearly documented. Thanks for that! =D

lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
 
   // 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
+    // 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));
+  }
+
   // 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::buildPerModuleDefaultPipeline(OptimizationLevel Level,
                                            bool DebugLogging) {
   assert(Level != O0 && "Must request optimizations for the default pipeline!");
 
   ModulePassManager MPM(DebugLogging);
 
   // Force any function attributes we want the rest of the pipeline to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
-  if (PGOOpt && PGOOpt->SamplePGOSupport) {
+  if (PGOOpt && PGOOpt->SamplePGOSupport)
     MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
-    if (!PGOOpt->SampleProfileFile.empty()) {
-      MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
-      MPM.addPass(PGOIndirectCallPromotion(false, true));
-    }
-  }
 
   // Add the core simplification pipeline.
   MPM.addPass(buildModuleSimplificationPipeline(Level, ThinLTOPhase::None,
                                                 DebugLogging));
 
   // Now add the optimization pipeline.
   MPM.addPass(buildModuleOptimizationPipeline(Level, DebugLogging));
 
   return MPM;
 }
 
 ModulePassManager
 PassBuilder::buildThinLTOPreLinkDefaultPipeline(OptimizationLevel Level,
                                                 bool DebugLogging) {
   assert(Level != O0 && "Must request optimizations for the default pipeline!");
 
   ModulePassManager MPM(DebugLogging);
 
   // Force any function attributes we want the rest of the pipeline to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
-  // Invoke the SamplePGO annotation pass for the first time to annotate
-  // profile for functions in the current module to give ThinLink info
-  // about module grouping.
-  if (PGOOpt && PGOOpt->SamplePGOSupport) {
+  if (PGOOpt && PGOOpt->SamplePGOSupport)
     MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
-    if (!PGOOpt->SampleProfileFile.empty())
-      MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
-  }
 
   // If we are planning to perform ThinLTO later, we don't bloat the code with
   // unrolling/vectorization/... now. Just simplify the module as much as we
   // can.
   MPM.addPass(buildModuleSimplificationPipeline(Level, ThinLTOPhase::PreLink,
                                                 DebugLogging));
 
   // Run partial inlining pass to partially inline functions that have
PassBuilder::buildThinLTODefaultPipeline(OptimizationLevel Level,
   // When it's going to be hooked, enable WholeProgramDevirt and LowerTypeTest
   // here.
 
   ModulePassManager MPM(DebugLogging);
 
   // Force any function attributes we want the rest of the pipeline to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
-  // Invoke the SamplePGO annotation pass for the second time to annotate on
-  // functions imported from other modules.
-  if (PGOOpt && !PGOOpt->SampleProfileFile.empty())
-    MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
-
   // During the ThinLTO backend phase we perform early indirect call promotion
   // here, before globalopt. Otherwise imported available_externally functions
   // look unreferenced and are removed.
-  MPM.addPass(PGOIndirectCallPromotion(
-      true /* InLTO */, PGOOpt && !PGOOpt->SampleProfileFile.empty()));
+  // FIXME: move this into buildModuleSimplificationPipeline to merge the logic
+  //        with SamplePGO.
+  if (PGOOpt && !PGOOpt->ProfileUseFile.empty())
+    MPM.addPass(PGOIndirectCallPromotion(true /* InLTO */,
+                                         false /* SamplePGO */));
 
   // Add the core simplification pipeline.
   MPM.addPass(buildModuleSimplificationPipeline(Level, ThinLTOPhase::PostLink,
                                                 DebugLogging));
 
   // Now add the optimization pipeline.
   MPM.addPass(buildModuleOptimizationPipeline(Level, DebugLogging));
 

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 |FileCheck %s --check-prefixes=SAMPLE_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: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
+; 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: 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
 }

test/Other/new-pm-thinlto-defaults.ll

 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-POSTLINK-O,CHECK-POSTLINK-O2
 ;
 ; CHECK-O: Starting llvm::Module pass manager run.
 ; CHECK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Module pass manager run.
 ; CHECK-O-NEXT: Running pass: ForceFunctionAttrsPass
 ; CHECK-DIS-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
 ; CHECK-DIS-NEXT: Running analysis: InnerAnalysisManagerProxy
-; CHECK-POSTLINK-O-NEXT: Running pass: PGOIndirectCallPromotion
-; CHECK-POSTLINK-O-NEXT: Running analysis: InnerAnalysisManagerProxy<{{.*}}Function
-; CHECK-POSTLINK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Module pass manager run.
 ; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-PRELINK-O-NODIS-NEXT: Running analysis: InnerAnalysisManagerProxy
+; CHECK-POSTLINK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
 ; CHECK-O-NEXT: Running pass: SROA
 ; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PromotePass>
 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-PRELINK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
+; CHECK-POSTLINK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}GlobalsAA
 ; CHECK-O-NEXT: Running analysis: GlobalsAA
 ; CHECK-O-NEXT: Running analysis: CallGraphAnalysis
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running pass: ModuleToPostOrderCGSCCPassAdaptor<{{.*}}LazyCallGraph{{.*}}>