[PM] Hook the instrumented PGO machinery in the new PM

[davide]

Summary

This is the required plumbing to add PGO to the new PM (which allows to test the changes to make the inliner profile-aware).
As long as several of us are interested in this, I decided to share it early (in the hope to save to somebody some boring typing).
I'll upload the clang bits soon, but I have some comments.

1. The default threshold for the inliner run used to be a tunable. I just hardcoded the value of the cl::opt. If people think it's valuable to thread this from the frontend down to the optimizer, happy to (but I wouldn't keep the cl::opt around). My take is that this option shouldn't be exposed.
2. There seems to be a comment about the hint threshold of the inliner. I blatantly cargo culted that from the old PM, but maybe should be removed ? (or we should get around to do the required performance testing to tune it)

Thanks!

[davidxl]

lib/Passes/PassBuilder.cpp

394 ↗

(On Diff #86349)

can you refactor the old pm code such that the existing internal options can be reused here?

487 ↗

(On Diff #86349)

this seems like exposing a bug in old implementation as well. IndirectCallPromotion pass need not to be run profile-use is in effect. Otherwise it is a waste of compile time.

[davide]

lib/Passes/PassBuilder.cpp

394 ↗

(On Diff #86349)

I'm under the impression that several (me included) dislike the use of cl::opt for the new pass manager. Do you have any suggestions on how to handle this haring without sharing the cl::opts ?

487 ↗

(On Diff #86349)

Will submit a patch separately. It's not gonna be a substantial reduction in compile time as this pass is very fast, but still there's no point in running it.

[chandlerc]

include/llvm/Passes/PassBuilder.h

279–282 ↗

(On Diff #86349)

I think we should extract this to a PGO option struct that is passed into the passBuilder rather than being public. What do you think?

lib/Passes/PassBuilder.cpp

383–384 ↗

(On Diff #86349)

Make this an assert and only call this routine when the conditions are met? It seems nicer to have the predicate in the caller so that the caller understands what enables or doesn't this pipeline.

386–390 ↗

(On Diff #86349)

I dunno why we would want to change this when optimizing for size really.... I mean, maybe disable this under Oz?

I'd write the comment about why it is disabled, not about the old pass manager.

394 ↗

(On Diff #86349)

I'm happy to have a cl::opt interface for things that we only expect to use as developers when testing / experimenting and we don't expect to support.

Inliner thresholds seem like a good example of this: we don't actually (IMO) want to expose a supported interface for arbitrarily changing the threshold because we're not going to test / support arbitrary thresholds. But we *do* want to be able to run experiments, etc.

However, this particular threshold seems an even less interesting thing to tune as it is chose more based on the nature of instrumentation rather than on any performance characteristics... So I don't feel strongly about whether it is a constant or a cl::opt.

As to whether we should share these, I again don't feel strongly. I'm happy to have two cl::opt flags if that's more convenient because, again, they're an internal interface used for development. However, if that's causing issues, we can add a helper function to the inliner along the lines of getPGOPreInlineParams() and sink these values (and their cl::opt flags) into that implementation so it can be shared between the two PMs.

[davide]

Your comments are reasonable, I'll address them soon.
@davidxl, @rongxu, @eraman what's the rationale behind not running the pre-inliner and simplification passes when optimizing for size?
My understanding is that we run them to cleanup and avoid putting "too many counters" (which would slow down the instrumentation) [classical example operator +].
Am I correct or is there any other reason?

[davide]

Actually, @xur, not @rongxu

[davide]

Updated + comments. I'll strip the call to indirectcallpromotion in the !PGO case as a follow-up.

include/llvm/Passes/PassBuilder.h

279–282 ↗

(On Diff #86349)

Done.

lib/Passes/PassBuilder.cpp

383–384 ↗

(On Diff #86349)

Moved the check.

386–390 ↗

(On Diff #86349)

I updated the comment, and maintained the old behaviour (not running simplifications for -Os/Oz).
I agree this could be re-evaluated in the future.

394 ↗

(On Diff #86349)

I agree with you. David, would you be fine with the status quo or you feel strong about having this a cl::opt ? [I personally don't think we should spend too much time worrying about sharing code between old and new PM in lib/Passes (we already share enough where it matters, e.g. in the passes themselves).

[davidxl]

lib/Passes/PassBuilder.cpp

394 ↗

(On Diff #86349)

keeping it as is is fine -- we can revisit this later if we want.

[chandlerc]

LGTM with a minor tweak above, feel free to just submit with that change unless you'd like to do something different.

include/llvm/Passes/PassBuilder.h

135–136 ↗

(On Diff #87712)

Pass and store an optional rather than a pointer? Removes lifetime concerns and ownersihp questions, and this seems a not-scary data type to copy.

llvm/trunk/include/llvm/Passes/PassBuilder.h

 #include "llvm/Transforms/Scalar/LoopPassManager.h"
 #include <vector>
 
 namespace llvm {
 class StringRef;
 class AAManager;
 class TargetMachine;
 
+/// A struct capturing PGO tunables.
+struct PGOOptions {
+  std::string ProfileGenFile = "";
+  std::string ProfileUseFile = "";
+  bool RunProfileGen = false;
+};
+
 /// \brief This class provides access to building LLVM's passes.
 ///
 /// It's members provide the baseline state available to passes during their
 /// construction. The \c PassRegistry.def file specifies how to construct all
 /// of the built-in passes, and those may reference these members during
 /// construction.
 class PassBuilder {
   TargetMachine *TM;
+  Optional<PGOOptions> PGOOpt;
 
 public:
   /// \brief LLVM-provided high-level optimization levels.
   ///
   /// This enumerates the LLVM-provided high-level optimization levels. Each
   /// level has a specific goal and rationale.
   enum OptimizationLevel {
     /// Disable as many optimizations as possible. This doesn't completely
enum OptimizationLevel {
     ///
     /// This is useful primarily when there are absolute size limitations and
     /// any effort taken to reduce the size is worth it regardless of the
     /// execution time impact. You should expect this level to produce rather
     /// slow, but very small, code.
     Oz
   };
 
-  explicit PassBuilder(TargetMachine *TM = nullptr) : TM(TM) {}
+  explicit PassBuilder(TargetMachine *TM = nullptr,
+                       Optional<PGOOptions> PGOOpt = None)
+      : TM(TM), PGOOpt(PGOOpt) {}
 
   /// \brief Cross register the analysis managers through their proxies.
   ///
   /// This is an interface that can be used to cross register each
   // AnalysisManager with all the others analysis managers.
   void crossRegisterProxies(LoopAnalysisManager &LAM,
                             FunctionAnalysisManager &FAM,
                             CGSCCAnalysisManager &CGAM,

llvm/trunk/lib/Passes/PassBuilder.cpp

#endif
   // the simplifications and basic cleanup after all the simplifications.
   FPM.addPass(ADCEPass());
   FPM.addPass(SimplifyCFGPass());
   FPM.addPass(InstCombinePass());
 
   return FPM;
 }
 
+static void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
+                              PassBuilder::OptimizationLevel Level,
+                              bool RunProfileGen, std::string ProfileGenFile,
+                              std::string ProfileUseFile) {
+  // 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.
+  if (!isOptimizingForSize(Level)) {
+    InlineParams IP;
+
+    // In the old pass manager, this is a cl::opt. Should still this be one?
+    IP.DefaultThreshold = 75;
+
+    // FIXME: The hint threshold has the same value used by the regular inliner.
+    // This should probably be lowered after performance testing.
+    // FIXME: this comment is cargo culted from the old pass manager, revisit).
+    IP.HintThreshold = 325;
+
+    CGSCCPassManager CGPipeline(DebugLogging);
+
+    CGPipeline.addPass(InlinerPass(IP));
+
+    FunctionPassManager FPM;
+    FPM.addPass(SROA());
+    FPM.addPass(EarlyCSEPass());    // Catch trivial redundancies.
+    FPM.addPass(SimplifyCFGPass()); // Merge & remove basic blocks.
+    FPM.addPass(InstCombinePass()); // Combine silly sequences.
+
+    // FIXME: Here the old pass manager inserts peephole extensions.
+    // Add them when they're supported.
+    CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));
+
+    MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));
+  }
+
+  if (RunProfileGen) {
+    MPM.addPass(PGOInstrumentationGen());
+
+    // Add the profile lowering pass.
+    InstrProfOptions Options;
+    if (!ProfileGenFile.empty())
+      Options.InstrProfileOutput = ProfileGenFile;
+    MPM.addPass(InstrProfiling(Options));
+  }
+
+  if (!ProfileUseFile.empty())
+    MPM.addPass(PGOInstrumentationUse(ProfileUseFile));
+}
+
 ModulePassManager
 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 te observe.
   MPM.addPass(ForceFunctionAttrsPass());
PassBuilder::buildPerModuleDefaultPipeline(OptimizationLevel Level,
 
   // Create a small function pass pipeline to cleanup after all the global
   // optimizations.
   FunctionPassManager GlobalCleanupPM(DebugLogging);
   GlobalCleanupPM.addPass(InstCombinePass());
   GlobalCleanupPM.addPass(SimplifyCFGPass());
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(GlobalCleanupPM)));
 
+  // Add all the requested passes for PGO Instrumentation, if requested.
+  if (PGOOpt) {
+    assert(PGOOpt->RunProfileGen || !PGOOpt->ProfileUseFile.empty());
+    addPGOInstrPasses(MPM, DebugLogging, Level, PGOOpt->RunProfileGen,
+                      PGOOpt->ProfileGenFile, PGOOpt->ProfileUseFile);
+  }
+
+  // Indirect call promotion that promotes intra-module targes only.
+  MPM.addPass(PGOIndirectCallPromotion());
+
   // Require the GlobalsAA analysis for the module so we can query it within
   // the CGSCC pipeline.
   MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
 
   // Now begin the main postorder CGSCC pipeline.
   // FIXME: The current CGSCC pipeline has its origins in the legacy pass
   // manager and trying to emulate its precise behavior. Much of this doesn't
   // make a lot of sense and we should revisit the core CGSCC structure.

llvm/trunk/test/Other/new-pm-defaults.ll

 ; 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-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
+; CHECK-O-NEXT: Running pass: PGOIndirectCallPromotion
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}GlobalsAA
 ; CHECK-O-NEXT: Running analysis: GlobalsAA
 ; CHECK-O-NEXT: Running analysis: CallGraphAnalysis
 ; CHECK-O-NEXT: Running pass: ModuleToPostOrderCGSCCPassAdaptor<{{.*}}LazyCallGraph{{.*}}>
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
 ; CHECK-O-NEXT: Starting CGSCC pass manager run.
 ; CHECK-O-NEXT: Running pass: InlinerPass