Update the new PM pipeline to make ICP aware if it is SamplePGO build.

[danielcdh]

Summary

In ThinLTO backend compile, OPTOptions are not set so that the ICP in ThinLTO backend does not know if it is a SamplePGO build, in which profile count needs to be annotated directly on call instructions. This patch cleaned up the PGOOptions handling logic and passes down PGOOptions to ThinLTO backend.

[chandlerc]

lib/Passes/PassBuilder.cpp

765–767

This is a *very* significant change in the PGO instrumentation pass ordering.

Before, we would do non-trivial cleanup of the IR module before adding instrumentation passes. Now they get added before any of that cleanup. This includes thing such as the first (and primary) SSA formation pass, lowering @llvm.expect intrinsics into branch weight metadata, dead argument elimination, global-opt, etc.

Was all of that intended? Is it really OK to instrument before those cleanups?

[davidxl]

Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread. This one should be left as SamplePGO only change.

[danielcdh]

lib/Passes/PassBuilder.cpp

765–767

There are still some optimizations invoked in addPGOInstrPasses before the actual instrumentation:

FunctionPassManager FPM;
FPM.addPass(SROA());
FPM.addPass(EarlyCSEPass());    // Catch trivial redundancies.
FPM.addPass(SimplifyCFGPass()); // Merge & remove basic blocks.
FPM.addPass(InstCombinePass()); // Combine silly sequences.
invokePeepholeEPCallbacks(FPM, Level);

CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));

MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));

If we invoke this in the original location, SROA/EarlyCSE/SimplifyCFG will be invoked twice before entering PGO instr/use pass, seems redundant to me. I've also tested the instrumentation PGO on our largest benchmark, and did not see performance change.

I know these EarlyFPM were to match the behavior of the legacy PM. But maybe it should not belong to buildModuleSimplificationPipeline, which is called by both regular pipeline, prelink, and thinlto default pipeline. At least thinlto default pipeline does not need these passes?

[danielcdh]

In D36052#825318, @davidxl wrote:

Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread. This one should be left as SamplePGO only change.

The tricky part is that PGOOptions was not passed in the ThinLTO backend before this patch. But after this patch, it will be passed in. However, in buildModuleSimplificationPipeline, it cannot know if this is the ThinLTO backend. As a result, we cannot disable instrumentation/annotation pass in there.

The potential solution could be:

• add another boolean value in buildModuleSimplificationPipeline to indicate it's ThinLTO backend
• refactor the code from the beginning of buildModuleSimplificationPipeline to the PGOOpt handling to a separate function, and call it separately in the caller of buildModuleSimplificationPipeline.

Any suggestions on how to move forward?

Thanks,
Dehao

[chandlerc]

In D36052#825321, @danielcdh wrote:

In D36052#825318, @davidxl wrote:

Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread. This one should be left as SamplePGO only change.

FWIW, I do agree with you Dehao that we seem to be deing redundant simplification, but as David says here we shouldn't change that behavior in this patch.

The tricky part is that PGOOptions was not passed in the ThinLTO backend before this patch. But after this patch, it will be passed in. However, in buildModuleSimplificationPipeline, it cannot know if this is the ThinLTO backend. As a result, we cannot disable instrumentation/annotation pass in there.

The potential solution could be:

• add another boolean value in buildModuleSimplificationPipeline to indicate it's ThinLTO backend
• refactor the code from the beginning of buildModuleSimplificationPipeline to the PGOOpt handling to a separate function, and call it separately in the caller of buildModuleSimplificationPipeline.

I actually think the second option might be interesting, but before we do that I think understanding how much redundancy there really is in the cleanup passes is important. That will help inform how to refactor these into separate components, or if there even *are* separate components. So I largely agree w/ David that something like #1 is the best path forward.

However, rather than adding another boolean value though, I think you should turn the current boolean into an enumeration for ThinLTO phase; "none" by default, and then with beth prelink and postlink values.

Note that I don't think we should call the phase "backend" or "codegen". Those are heavily overloaded in LLVM, so I think pre/post-link is a better terminology to use.

-Chandler

[danielcdh]

In D36052#825325, @chandlerc wrote:

In D36052#825321, @danielcdh wrote:

In D36052#825318, @davidxl wrote:

Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread. This one should be left as SamplePGO only change.

FWIW, I do agree with you Dehao that we seem to be deing redundant simplification, but as David says here we shouldn't change that behavior in this patch.

The tricky part is that PGOOptions was not passed in the ThinLTO backend before this patch. But after this patch, it will be passed in. However, in buildModuleSimplificationPipeline, it cannot know if this is the ThinLTO backend. As a result, we cannot disable instrumentation/annotation pass in there.

The potential solution could be:

• add another boolean value in buildModuleSimplificationPipeline to indicate it's ThinLTO backend
• refactor the code from the beginning of buildModuleSimplificationPipeline to the PGOOpt handling to a separate function, and call it separately in the caller of buildModuleSimplificationPipeline.

I actually think the second option might be interesting, but before we do that I think understanding how much redundancy there really is in the cleanup passes is important. That will help inform how to refactor these into separate components, or if there even *are* separate components. So I largely agree w/ David that something like #1 is the best path forward.

However, rather than adding another boolean value though, I think you should turn the current boolean into an enumeration for ThinLTO phase; "none" by default, and then with beth prelink and postlink values.

Note that I don't think we should call the phase "backend" or "codegen". Those are heavily overloaded in LLVM, so I think pre/post-link is a better terminology to use.

Thanks Chandler and David for the suggestion.

I've sent a separate patch https://reviews.llvm.org/D36053 to refactor the code. The patch is NFC, but I'd like to ask for your opinions on the naming/description to see if it's concise. Thanks in advance for comments.

[danielcdh]

rebase and make instrumentation PGO behavior unchanged.

[chandlerc]

Looks awesome, LGTM.

This does move ICP to pre-cleanup passes for non-ThinLTO builds with sample PGO. I assume you've tested it and this is fine. (It also moves the sample profile loading, but that one seems completely innocuous to me.)

[danielcdh]

In D36052#825377, @chandlerc wrote:

Looks awesome, LGTM.

This does move ICP to pre-cleanup passes for non-ThinLTO builds with sample PGO. I assume you've tested it and this is fine. (It also moves the sample profile loading, but that one seems completely innocuous to me.)

Yes, verified that this does not affect non-ThinLTO performance. commit the patch now.

lib/LTO/LTOBackend.cpp

else
     RelocModel =
         M.getPICLevel() == PICLevel::NotPIC ? Reloc::Static : Reloc::PIC_;
 
   return std::unique_ptr<TargetMachine>(TheTarget->createTargetMachine(
       TheTriple, Conf.CPU, Features.getString(), Conf.Options, RelocModel,
       Conf.CodeModel, Conf.CGOptLevel));
 }
 
-static void runNewPMPasses(Module &Mod, TargetMachine *TM, unsigned OptLevel,
-                           bool IsThinLTO) {
-  PassBuilder PB(TM);
+static void runNewPMPasses(Config &Conf, Module &Mod, TargetMachine *TM,
+                           unsigned OptLevel, bool IsThinLTO) {
+  Optional<PGOOptions> PGOOpt;
+  if (!Conf.SampleProfile.empty())
+    PGOOpt = PGOOptions("", "", Conf.SampleProfile, false, true);
+
+  PassBuilder PB(TM, PGOOpt);
   AAManager AA;
 
   // Parse a custom AA pipeline if asked to.
   assert(PB.parseAAPipeline(AA, "default"));
 
   LoopAnalysisManager LAM;
   FunctionAnalysisManager FAM;
   CGSCCAnalysisManager CGAM;

 bool opt(Config &Conf, TargetMachine *TM, unsigned Task, Module &Mod,
          bool IsThinLTO, ModuleSummaryIndex *ExportSummary,
          const ModuleSummaryIndex *ImportSummary) {
   // FIXME: Plumb the combined index into the new pass manager.
   if (!Conf.OptPipeline.empty())
     runNewPMCustomPasses(Mod, TM, Conf.OptPipeline, Conf.AAPipeline,
                          Conf.DisableVerify);
   else if (Conf.UseNewPM)
-    runNewPMPasses(Mod, TM, Conf.OptLevel, IsThinLTO);
+    runNewPMPasses(Conf, Mod, TM, Conf.OptLevel, IsThinLTO);
   else
     runOldPMPasses(Conf, Mod, TM, IsThinLTO, ExportSummary, ImportSummary);
   return !Conf.PostOptModuleHook || Conf.PostOptModuleHook(Task, Mod);
 }
 
 void codegen(Config &Conf, TargetMachine *TM, AddStreamFn AddStream,
              unsigned Task, Module &Mod) {
   if (Conf.PreCodeGenModuleHook && !Conf.PreCodeGenModuleHook(Task, Mod))

lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   // optimizations.
   FunctionPassManager GlobalCleanupPM(DebugLogging);
   GlobalCleanupPM.addPass(InstCombinePass());
   invokePeepholeEPCallbacks(GlobalCleanupPM, Level);
 
   GlobalCleanupPM.addPass(SimplifyCFGPass());
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(GlobalCleanupPM)));
 
-  // Add all the requested passes for PGO, if requested.
-  if (PGOOpt) {
-    if (!PGOOpt->ProfileGenFile.empty() || !PGOOpt->ProfileUseFile.empty())
-      // Instrumentation based PGO (gen and use)
-      addPGOInstrPasses(MPM, DebugLogging, Level, PGOOpt->RunProfileGen,
-                        PGOOpt->ProfileGenFile, PGOOpt->ProfileUseFile);
-    else if (!PGOOpt->SampleProfileFile.empty())
-      // SamplePGO use
-      MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
-
-    // Indirect call promotion that promotes intra-module targes only.
-    // Do not enable it in PrepareForThinLTO phase during sample PGO because
-    // it changes IR to makes profile annotation in back compile inaccurate.
-    if ((!PrepareForThinLTO && !PGOOpt->SampleProfileFile.empty())
-        || !PGOOpt->ProfileUseFile.empty())
-      MPM.addPass(PGOIndirectCallPromotion(
-          false, PGOOpt && !PGOOpt->SampleProfileFile.empty()));
-  }
-
   // Require the GlobalsAA analysis for the module so we can query it within
   // the CGSCC pipeline.
   MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
 
   // Require the ProfileSummaryAnalysis for the module so we can query it within
   // the inliner pass.
   MPM.addPass(RequireAnalysisPass<ProfileSummaryAnalysis, Module>());
 
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) {
+    if (PGOOpt->SamplePGOSupport)
       MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
+    if (!PGOOpt->ProfileGenFile.empty() || !PGOOpt->ProfileUseFile.empty())
+      addPGOInstrPasses(MPM, DebugLogging, Level, PGOOpt->RunProfileGen,
+                        PGOOpt->ProfileGenFile, PGOOpt->ProfileUseFile);
+    else if (!PGOOpt->SampleProfileFile.empty())
+      MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
+    if (!PGOOpt->ProfileUseFile.empty() || !PGOOpt->SampleProfileFile.empty())
+      MPM.addPass(PGOIndirectCallPromotion(
+          false, !PGOOpt->SampleProfileFile.empty()));
+  }
 
   // Add the core simplification pipeline.
   MPM.addPass(buildModuleSimplificationPipeline(Level, DebugLogging,
                                                 /*PrepareForThinLTO=*/false));
 
   // 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());
 
-  if (PGOOpt && PGOOpt->SamplePGOSupport)
+  if (PGOOpt) {
+    if (PGOOpt->SamplePGOSupport)
       MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
 
+    if (!PGOOpt->ProfileGenFile.empty() || !PGOOpt->ProfileUseFile.empty()) {
+      addPGOInstrPasses(MPM, DebugLogging, Level, PGOOpt->RunProfileGen,
+                        PGOOpt->ProfileGenFile, PGOOpt->ProfileUseFile);
+      if (!PGOOpt->ProfileUseFile.empty())
+        MPM.addPass(PGOIndirectCallPromotion(false, false));
+    } else 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, DebugLogging,
                                                 /*PrepareForThinLTO=*/true));
 
   // Run partial inlining pass to partially inline functions that have
   // large bodies.
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 SampleProfileLoader the 2nd time to annotate profile.
+  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() &&
-                            !PGOOpt->ProfileUseFile.empty()));
+      true /* InLTO */, PGOOpt && !PGOOpt->SampleProfileFile.empty()));
 
   // Add the core simplification pipeline.
   MPM.addPass(buildModuleSimplificationPipeline(Level, DebugLogging,
                                                 /*PrepareForThinLTO=*/false));
 
   // Now add the optimization pipeline.
   MPM.addPass(buildModuleOptimizationPipeline(Level, DebugLogging));
 

test/LTO/Resolution/X86/Inputs/load-sample-prof-icp.prof

+test:1000:0
+ 1: 1000 bar:1000

test/LTO/Resolution/X86/load-sample-prof-icp.ll

+; Test that llvm-lto2 handles the -lto-sample-profile-file option and pass
+; down to the ICP correctly.
+;
+; RUN: opt -module-summary < %s -o %t.bc
+; RUN: llvm-lto2 run -o %t.out %t.bc -save-temps \
+; RUN:   -r %t.bc,test,px -r %t.bc,bar,x \
+; RUN:   -lto-sample-profile-file=%S/Inputs/load-sample-prof-icp.prof
+; RUN: llvm-dis %t.out.0.4.opt.bc -o - | FileCheck %s
+; RUN: llvm-lto2 run -o %t.out %t.bc -save-temps \
+; RUN:   -r %t.bc,test,px -r %t.bc,bar,x -use-new-pm \
+; RUN:   -lto-sample-profile-file=%S/Inputs/load-sample-prof-icp.prof
+; RUN: llvm-dis %t.out.0.4.opt.bc -o - | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-LABEL: @test
+; Checks that the call instruction is promoted to direct call and has
+; profile count annotated on the direct call.
+define void @test(void ()*) !dbg !7 {
+  %2 = alloca void ()*
+  store void ()* %0, void ()** %2
+  %3 = load void ()*, void ()** %2
+  ; CHECK: call void @bar(),{{.*}}!prof
+  call void %3(), !dbg !10
+  ret void
+}
+
+declare void @bar() local_unnamed_addr
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 6.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 6.0.0 "}
+!7 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 3, type: !8, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: true, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+!10 = !DILocation(line: 4, column: 5, scope: !7)
+!11 = !DILocation(line: 5, column: 1, scope: !7)