[HotColdSplit] Move splitting after instrumented PGO use

Summary:
Follow up to D57082 which moved splitting earlier in the pipeline, in
order to perform it before inlining. However, it was moved too early,
before the IR is annotated with instrumented PGO data. This caused the
splitting to incorrectly determine cold functions.

Move it to just after PGO annotation (still before inlining), in both
pass managers.

Reviewers: vsk, hiraditya, sebpop

Subscribers: mehdi_amini, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D57805

llvm-svn: 353270

5 files changed, 32 insertions, 13 deletions

diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
index aa82f268338..8e62c1d0b69 100644
--- a/llvm/lib/Passes/PassBuilder.cpp
+++ b/llvm/lib/Passes/PassBuilder.cpp
@@ -680,14 +680,6 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   // globals.
   MPM.addPass(DeadArgumentEliminationPass());
 
-  // Split out cold code. Splitting is done before inlining because 1) the most
-  // common kinds of cold regions can (a) be found before inlining and (b) do
-  // not grow after inlining, and 2) inhibiting inlining of cold code improves
-  // code size & compile time. Split after Mem2Reg to make code model estimates
-  // more accurate, but before InstCombine to allow it to clean things up.
-  if (EnableHotColdSplit && Phase != ThinLTOPhase::PostLink)
-    MPM.addPass(HotColdSplittingPass());
-
   // Create a small function pass pipeline to cleanup after all the global
   // optimizations.
   FunctionPassManager GlobalCleanupPM(DebugLogging);
@@ -710,6 +702,14 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   if (EnableSyntheticCounts && !PGOOpt)
     MPM.addPass(SyntheticCountsPropagation());
 
+  // Split out cold code. Splitting is done before inlining because 1) the most
+  // common kinds of cold regions can (a) be found before inlining and (b) do
+  // not grow after inlining, and 2) inhibiting inlining of cold code improves
+  // code size & compile time. Split after Mem2Reg to make code model estimates
+  // more accurate, but before InstCombine to allow it to clean things up.
+  if (EnableHotColdSplit && Phase != ThinLTOPhase::PostLink)
+    MPM.addPass(HotColdSplittingPass());
+
   // Require the GlobalsAA analysis for the module so we can query it within
   // the CGSCC pipeline.
   MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
diff --git a/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp b/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
index 03d7088eab4..8f2860ba51b 100644
--- a/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
+++ b/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
@@ -517,11 +517,6 @@ void PassManagerBuilder::populateModulePassManager(
 
   MPM.add(createDeadArgEliminationPass()); // Dead argument elimination
 
-  // Split out cold code before inlining. See comment in the new PM
-  // (\ref buildModuleSimplificationPipeline).
-  if (EnableHotColdSplit && DefaultOrPreLinkPipeline)
-    MPM.add(createHotColdSplittingPass());
-
   addInstructionCombiningPass(MPM); // Clean up after IPCP & DAE
   addExtensionsToPM(EP_Peephole, MPM);
   MPM.add(createCFGSimplificationPass()); // Clean up after IPCP & DAE
@@ -534,6 +529,11 @@ void PassManagerBuilder::populateModulePassManager(
   if (DefaultOrPreLinkPipeline && !PrepareForThinLTOUsingPGOSampleProfile)
     addPGOInstrPasses(MPM);
 
+  // Split out cold code before inlining. See comment in the new PM
+  // (\ref buildModuleSimplificationPipeline).
+  if (EnableHotColdSplit && DefaultOrPreLinkPipeline)
+    MPM.add(createHotColdSplittingPass());
+
   // We add a module alias analysis pass here. In part due to bugs in the
   // analysis infrastructure this "works" in that the analysis stays alive
   // for the entire SCC pass run below.
diff --git a/llvm/test/Other/Inputs/pass-pipelines.proftext b/llvm/test/Other/Inputs/pass-pipelines.proftext
new file mode 100644
index 00000000000..04a7c1c1a35
--- /dev/null
+++ b/llvm/test/Other/Inputs/pass-pipelines.proftext
@@ -0,0 +1 @@
+:ir
diff --git a/llvm/test/Other/new-pm-pgo.ll b/llvm/test/Other/new-pm-pgo.ll
index c1a26b449c1..916309e625b 100644
--- a/llvm/test/Other/new-pm-pgo.ll
+++ b/llvm/test/Other/new-pm-pgo.ll
@@ -1,6 +1,7 @@
 ; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-kind=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=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE
+; RUN: opt -debug-pass-manager -passes='default<O2>' -hot-cold-split -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE --check-prefixes=SPLIT
 ; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-kind=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=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-pgo.prof' %s 2>&1 \
@@ -12,6 +13,7 @@
 ; GEN: Running pass: PGOInstrumentationGen
 ; USE: Running pass: PGOInstrumentationUse
 ; USE: Running pass: PGOIndirectCallPromotion
+; SPLIT: Running pass: HotColdSplittingPass
 ; USE: Running pass: PGOMemOPSizeOpt
 ; SAMPLE_USE_O: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
 ; SAMPLE_USE_PRE_LINK: Running pass: ModuleToFunctionPassAdaptor<{{.*}}AddDiscriminatorsPass{{.*}}>
diff --git a/llvm/test/Other/pass-pipelines.ll b/llvm/test/Other/pass-pipelines.ll
index dddf1338a4d..dfddcee55e8 100644
--- a/llvm/test/Other/pass-pipelines.ll
+++ b/llvm/test/Other/pass-pipelines.ll
@@ -6,6 +6,16 @@
 ; RUN: opt -disable-output -disable-verify -debug-pass=Structure \
 ; RUN:     -O2 %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-O2
+; RUN: llvm-profdata merge %S/Inputs/pass-pipelines.proftext -o %t.profdata
+; RUN: opt -disable-output -disable-verify -debug-pass=Structure \
+; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
+; RUN:     -O2 %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-O2 --check-prefix=PGOUSE
+; RUN: opt -disable-output -disable-verify -debug-pass=Structure \
+; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
+; RUN:     -hot-cold-split \
+; RUN:     -O2 %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-O2 --check-prefix=PGOUSE --check-prefix=SPLIT
 ;
 ; In the first pipeline there should just be a function pass manager, no other
 ; pass managers.
@@ -27,6 +37,12 @@
 ; Very carefully assert the CGSCC pass pipeline as it is fragile and unusually
 ; susceptible to phase ordering issues.
 ; CHECK-O2: CallGraph Construction
+; PGOUSE: Call Graph SCC Pass Manager
+; PGOUSE:      Function Integration/Inlining
+; PGOUSE: PGOInstrumentationUsePass
+; PGOUSE: PGOIndirectCallPromotion
+; SPLIT: Hot Cold Splitting
+; PGOUSE: CallGraph Construction
 ; CHECK-O2-NEXT: Globals Alias Analysis
 ; CHECK-O2-NEXT: Call Graph SCC Pass Manager
 ; CHECK-O2-NEXT: Remove unused exception handling info