[PGO] Enable preinline and cleanup when optimize for size

[zequanwu]

Summary

This is intended to reduce the binary size of both phase 1 and phase 2 builds.
By compiling chrome on Windows (using -Os in both phases), the binary size of phase 1 is reduced by 27.5% and binary size of phase 2 is reduced by 8.4%.

[zequanwu]

update wrong diff.

[xur]

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz (SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the preinliner is pretty conservative. But there will be cases size will be increased.

I would like more test results (like bootstrap clang) before committing.

[rnk]

Zequan, to build clang with PGO, you can follow the steps in Chrome's script to build clang with PGO:
https://source.chromium.org/chromium/chromium/src/+/master:tools/clang/scripts/build.py;l=703?q=clang%2Fscripts%2F%20build.py&ss=chromium

Regarding -Oz / minsize / SizeLevel 2, what we have discovered is that *not* running the preinliner makes the instrumented binary almost unusably large. Even if 75 is the wrong inline threshold for minsize, it's better than not running the preinliner. I think we should run with it.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

334

This code is duplicated for the NPM and the old PM. Please keep them in sync.

[zequanwu]

In D91673#2401578, @xur wrote:

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz (SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the preinliner is pretty conservative. But there will be cases size will be increased.

I would like more test results (like bootstrap clang) before committing.

I don't see binary size difference (<1%), when bootstrap clang with this change. I was using steps at https://llvm.org/docs/HowToBuildWithPGO.html#building-clang-with-pgo.

[zequanwu]

• sync NPM with LPM.
• use PreInlineThreshold flag value instead of hardcode number.

[rnk]

Thanks, my concerns are addressed, and I believe @xur's are as well. Please wait for his input before landing, though.

[xur]

Sorry to the late response (I somehow missed the reply).

(1) Can we add some comments to explain the reason for enabling this for -Oz -- I think the comments from @rnk is useful to some people (at least to me).

(2) Also please add a testcase.

Feel free to commit to address the above two issues.

[rnk]

I see two pre-commit test failures on the patch. Can you take a look at those as well?

[zequanwu]

• fix existing failed testcase.
• add new testcases for npm and lpm.
• add comments.

llvm/lib/Passes/PassBuilder.cpp

 }
 
 void PassBuilder::addPGOInstrPasses(ModulePassManager &MPM,
                                     PassBuilder::OptimizationLevel Level,
                                     bool RunProfileGen, bool IsCS,
                                     std::string ProfileFile,
                                     std::string ProfileRemappingFile) {
   assert(Level != OptimizationLevel::O0 && "Not expecting O0 here!");
-  // Generally running simplification passes and the inliner with an high
-  // threshold results in smaller executables, but there may be cases where
-  // the size grows, so let's be conservative here and skip this simplification
-  // at -Os/Oz. We will not do this  inline for context sensistive PGO (when
-  // IsCS is true).
-  if (!Level.isOptimizingForSize() && !IsCS && !DisablePreInliner) {
+  if (!IsCS && !DisablePreInliner) {
     InlineParams IP;
 
     IP.DefaultThreshold = PreInlineThreshold;
 
-    // FIXME: The hint threshold has the same value used by the regular inliner.
-    // This should probably be lowered after performance testing.
+    // FIXME: The hint threshold has the same value used by the regular inliner
+    // when not optimzing for size. This should probably be lowered after
+    // performance testing.
     // FIXME: this comment is cargo culted from the old pass manager, revisit).
-    IP.HintThreshold = 325;
+    IP.HintThreshold = Level.isOptimizingForSize() ? PreInlineThreshold : 325;
     ModuleInlinerWrapperPass MIWP(IP, DebugLogging);
     CGSCCPassManager &CGPipeline = MIWP.getPM();
 
     FunctionPassManager FPM;
     FPM.addPass(SROA());
     FPM.addPass(EarlyCSEPass());    // Catch trivial redundancies.
     FPM.addPass(SimplifyCFGPass()); // Merge & remove basic blocks.
     FPM.addPass(InstCombinePass()); // Combine silly sequences.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

void PassManagerBuilder::addPGOInstrPasses(legacy::PassManagerBase &MPM,
                                            bool IsCS = false) {
   if (IsCS) {
     if (!EnablePGOCSInstrGen && !EnablePGOCSInstrUse)
       return;
   } else if (!EnablePGOInstrGen && PGOInstrUse.empty() && PGOSampleUse.empty())
     return;
 
   // Perform the preinline and cleanup passes for O1 and above.
-  // And avoid doing them if optimizing for size.
   // We will not do this inline for context sensitive PGO (when IsCS is true).
-  if (OptLevel > 0 && SizeLevel == 0 && !DisablePreInliner &&
-      PGOSampleUse.empty() && !IsCS) {
+  if (OptLevel > 0 && !DisablePreInliner && PGOSampleUse.empty() && !IsCS) {
     // Create preinline pass. We construct an InlineParams object and specify
     // the threshold here to avoid the command line options of the regular
     // inliner to influence pre-inlining. The only fields of InlineParams we
     // care about are DefaultThreshold and HintThreshold.
     InlineParams IP;
     IP.DefaultThreshold = PreInlineThreshold;
-    // FIXME: The hint threshold has the same value used by the regular inliner.
-    // This should probably be lowered after performance testing.
-    IP.HintThreshold = 325;
+    // FIXME: The hint threshold has the same value used by the regular inliner
+    // when not optimzing for size. This should probably be lowered after
+    // performance testing.
+    // Use PreInlineThreshold for both -Os and -Oz. Not running preinliner makes
+    // the instrumented binary unusably large. Even if PreInlineThreshold is not
+    // correct thresold for -Oz, it is better than not running preinliner.
+    IP.HintThreshold = SizeLevel > 0 ? PreInlineThreshold : 325;
 
     MPM.add(createFunctionInliningPass(IP));
     MPM.add(createSROAPass());
     MPM.add(createEarlyCSEPass());             // Catch trivial redundancies
     MPM.add(createCFGSimplificationPass());    // Merge & remove BBs
     MPM.add(createInstructionCombiningPass()); // Combine silly seq's
     addExtensionsToPM(EP_Peephole, MPM);
   }

llvm/test/Other/new-pm-pgo-preinline.ll

+; RUN: opt -disable-verify -debug-pass-manager -pgo-kind=pgo-instr-gen-pipeline -passes='default<Os>' -S %s 2>&1 | FileCheck %s --check-prefixes=CHECK-Osz
+; RUN: opt -disable-verify -debug-pass-manager -pgo-kind=pgo-instr-gen-pipeline -passes='default<Oz>' -S %s 2>&1 | FileCheck %s --check-prefixes=CHECK-Osz
+
+; CHECK-Osz: Running pass: ModuleInlinerWrapperPass
+; CHECK-Osz-NEXT: Running analysis: InlineAdvisorAnalysis
+; CHECK-Osz-NEXT: Starting {{.*}}Module pass manager run.
+; CHECK-Osz-NEXT: Running analysis: InnerAnalysisManagerProxy
+; CHECK-Osz-NEXT: Running analysis: LazyCallGraphAnalysis
+; CHECK-Osz-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy on (foo)
+; CHECK-Osz-NEXT: Running analysis: OuterAnalysisManagerProxy
+; CHECK-Osz-NEXT: Starting CGSCC pass manager run.
+; CHECK-Osz-NEXT: Running pass: InlinerPass on (foo)
+; CHECK-Osz-NEXT: Running pass: SROA on foo
+; CHECK-Osz-NEXT: Running pass: EarlyCSEPass on foo
+; CHECK-Osz-NEXT: Running pass: SimplifyCFGPass on foo
+; CHECK-Osz-NEXT: Running pass: InstCombinePass on foo
+; CHECK-Osz-NEXT: Finished CGSCC pass manager run.
+; CHECK-Osz-NEXT: Finished {{.*}}Module pass manager run.
+; CHECK-Osz-NEXT: Running pass: GlobalDCEPass
+; CHECK-Osz-NEXT: Running pass: PGOInstrumentationGen
+
+define void @foo() {
+  ret void
+}

llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll

 ; Validate ThinLTO prelink pipeline when we have instrumentation PGO
 ;
 ; RUN: llvm-profdata merge %S/Inputs/new-pm-thinlto-prelink-pgo-defaults.proftext -o %t.profdata
 ;
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
 ; RUN:     -passes='thinlto-pre-link<O1>' -S %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-O123
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-O123SZ
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
 ; RUN:     -passes='thinlto-pre-link<O2>' -S  %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-O23SZ,CHECK-O123
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-O23SZ,CHECK-O123SZ
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
 ; RUN:     -passes='thinlto-pre-link<O3>' -S -passes-ep-pipeline-start='no-op-module' %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-O23SZ,CHECK-O123,CHECK-EP-PIPELINE-START
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-O23SZ,CHECK-O123SZ,CHECK-EP-PIPELINE-START
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
 ; RUN:     -passes='thinlto-pre-link<Os>' -S %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Os,CHECK-O23SZ
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O123SZ,CHECK-Os,CHECK-O23SZ
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
 ; RUN:     -passes='thinlto-pre-link<Oz>' -S %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-O23SZ
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O123SZ,CHECK-Oz,CHECK-O23SZ
 ; RUN: opt -disable-verify -debug-pass-manager -new-pm-debug-info-for-profiling \
 ; RUN:     -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' \
 ; RUN:     -passes='thinlto-pre-link<O2>' -S  %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-O23SZ,CHECK-O123
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-O23SZ,CHECK-O123SZ
 ;
 ; CHECK-O: Starting {{.*}}Module pass manager run.
 ; CHECK-O-NEXT: Running pass: Annotation2Metadata
 ; CHECK-O-NEXT: Running pass: ForceFunctionAttrsPass
 ; CHECK-EP-PIPELINE-START-NEXT: Running pass: NoOpModulePass
 ; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis

 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Starting {{.*}}Function pass manager run.
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running analysis: AAManager
 ; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Finished {{.*}}Function pass manager run.
-; CHECK-O123-NEXT: Running pass: ModuleInlinerWrapperPass
-; CHECK-O123-NEXT: Running analysis: InlineAdvisorAnalysis
-; CHECK-O123-NEXT: Starting {{.*}}Module pass manager run.
-; CHECK-O123-NEXT: Running analysis: InnerAnalysisManagerProxy
-; CHECK-O123-NEXT: Running analysis: LazyCallGraphAnalysis
-; CHECK-O123-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy on (foo)
-; CHECK-O123-NEXT: Running analysis: OuterAnalysisManagerProxy
-; CHECK-O123-NEXT: Starting CGSCC pass manager run.
-; CHECK-O123-NEXT: Running pass: InlinerPass on (foo)
-; CHECK-O123-NEXT: Running pass: SROA on foo
-; CHECK-O123-NEXT: Running pass: EarlyCSEPass on foo
-; CHECK-O123-NEXT: Running pass: SimplifyCFGPass on foo
-; CHECK-O123-NEXT: Running pass: InstCombinePass on foo
-; CHECK-O123-NEXT: Finished CGSCC pass manager run.
-; CHECK-O123-NEXT: Finished {{.*}}Module pass manager run.
-; CHECK-O123-NEXT: Running pass: GlobalDCEPass
+; CHECK-O123SZ-NEXT: Running pass: ModuleInlinerWrapperPass
+; CHECK-O123SZ-NEXT: Running analysis: InlineAdvisorAnalysis
+; CHECK-O123SZ-NEXT: Starting {{.*}}Module pass manager run.
+; CHECK-O123SZ-NEXT: Running analysis: InnerAnalysisManagerProxy
+; CHECK-O123SZ-NEXT: Running analysis: LazyCallGraphAnalysis
+; CHECK-O123SZ-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy on (foo)
+; CHECK-O123SZ-NEXT: Running analysis: OuterAnalysisManagerProxy
+; CHECK-O123SZ-NEXT: Starting CGSCC pass manager run.
+; CHECK-O123SZ-NEXT: Running pass: InlinerPass on (foo)
+; CHECK-O123SZ-NEXT: Running pass: SROA on foo
+; CHECK-O123SZ-NEXT: Running pass: EarlyCSEPass on foo
+; CHECK-O123SZ-NEXT: Running pass: SimplifyCFGPass on foo
+; CHECK-O123SZ-NEXT: Running pass: InstCombinePass on foo
+; CHECK-O123SZ-NEXT: Finished CGSCC pass manager run.
+; CHECK-O123SZ-NEXT: Finished {{.*}}Module pass manager run.
+; CHECK-O123SZ-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: PGOInstrumentationUse
 ; These next two can appear in any order since they are accessed as parameters
 ; on the same call to BlockFrequencyInfo::calculate.
 ; CHECK-O-DAG: Running analysis: BranchProbabilityAnalysis on foo
 ; CHECK-O-DAG: Running analysis: PostDominatorTreeAnalysis on foo
 ; CHECK-O-DAG: Running analysis: LoopAnalysis on foo
 ; CHECK-O-NEXT: Running analysis: BlockFrequencyAnalysis on foo
 ; CHECK-O-NEXT: Invalidating analysis: InnerAnalysisManagerProxy
-; CHECK-O123-NEXT: Invalidating analysis: LazyCallGraphAnalysis on
-; CHECK-O123-NEXT: Invalidating analysis: InnerAnalysisManagerProxy
+; CHECK-O123SZ-NEXT: Invalidating analysis: LazyCallGraphAnalysis on
+; CHECK-O123SZ-NEXT: Invalidating analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running pass: PGOIndirectCallPromotion on
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis on foo
 ; CHECK-O-NEXT: Running pass: ModuleInlinerWrapperPass
-; CHECK-Os-NEXT: Running analysis: InlineAdvisorAnalysis
-; CHECK-Oz-NEXT: Running analysis: InlineAdvisorAnalysis
 ; CHECK-O-NEXT: Starting {{.*}}Module pass manager run.
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis on foo
 ; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
 ; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph::SCC{{.*}}>
 ; CHECK-O-NEXT: Running pass: DevirtSCCRepeatedPass
 ; CHECK-O-NEXT: Starting CGSCC pass manager run.

llvm/test/Other/pm-pgo-preinline.ll

+; RUN: opt -disable-verify -enable-new-pm=0 -pgo-kind=pgo-instr-gen-pipeline -mtriple=x86_64-- -Os -debug-pass=Structure < %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-Osz
+; RUN: opt -disable-verify -enable-new-pm=0 -pgo-kind=pgo-instr-gen-pipeline -mtriple=x86_64-- -Oz -debug-pass=Structure < %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-Osz
+
+
+; CHECK-Osz: CallGraph Construction
+; CHECK-Osz-NEXT: Call Graph SCC Pass Manager
+; CHECK-Osz-NEXT: Function Integration/Inlining
+; CHECK-Osz-NEXT: FunctionPass Manager
+; CHECK-Osz-NEXT: Dominator Tree Construction
+; CHECK-Osz-NEXT: SROA
+; CHECK-Osz-NEXT: Early CSE
+; CHECK-Osz-NEXT: Simplify the CFG
+; CHECK-Osz-NEXT: Dominator Tree Construction
+; CHECK-Osz-NEXT: Basic Alias Analysis (stateless AA impl)
+; CHECK-Osz-NEXT: Function Alias Analysis Results
+; CHECK-Osz-NEXT: Natural Loop Information
+; CHECK-Osz-NEXT: Lazy Branch Probability Analysis
+; CHECK-Osz-NEXT: Lazy Block Frequency Analysis
+; CHECK-Osz-NEXT: Optimization Remark Emitter
+; CHECK-Osz-NEXT: Combine redundant instructions
+
+define void @foo() {
+  ret void
+}