Add test coverage for new PM PGOOpt handling.

[danielcdh]

Summary

This patch adds flags and tests to cover the PGOOpt handling logic in new PM.

[davide]

This is a step in the right direction, some comments inline.

lib/Passes/PassBuilder.cpp

1109

s/Use/Process/ ?

1116

Do you need return after fatal?

1118

Capital P

1120–1135

StringSwitch maybe?

[danielcdh]

address David's comments.

[chandlerc]

Awesome, I'm glad to see a nice way of testing this at a low level!

lib/Passes/PassBuilder.cpp

175–178

Rather than these options going in PassBuilder.cpp and overriding things, I think this should go into tools/opt/NewPMDriver.cpp much like the ThinLTO controls do.

Then the opt tool can construct a PGOOpt struct and hand it into the PassBuilder much the same way Clang will.

Also, sample_gen doesn't appear to be valid?

I think rather than having one flag that you have to parse I would suggest have three flags: pgo-instr-gen, pgo-intsr-use, and pgo-sample-use. I think there is a way in the cl::opt layer to say the flags are mutually incompatible, but even if you have to do that manually, it seems better than parsing an '=' out of the flag value.

[danielcdh]

update

[danielcdh]

lib/Passes/PassBuilder.cpp

175–178

pgo-instr-{gen|use}, conflict with existing flags. Used pgo-{gen|use|sample-use} instead.

[chandlerc]

Minor flag tweak...

tools/opt/NewPMDriver.cpp

168–170 ↗

(On Diff #108167)

What about a simpler factoring of flags:

cl::opt<PGOKind> PGOKindFlag(
    cl::values(clEnumValN(PGOKind::InstrGen, "new-pm-pgo-instr-gen-pipeline", "..."),
               clEnumValN(PGOKind::InstrUse, "new-pm-pgo-instr-use-pipeline", "..."),
               clEnumValN(PGOKind::SampleUse, "new-pm-pgo-sample-use-pipeline", "...")));

cl::opt<std::string> ProfileFile(...);

This would do the checking that only one kind is selected at a time for you I think.

[danielcdh]

update

[chandlerc]

I think the command line flags can be made simpler, but I must not be explaining it well in review. I suggest you go ahead and submit this and I'll immediately send you a follow-up patch that hopefully illustrates what I'm suggesting for handling the flags. And that will let you rebase and continue on other things.

[danielcdh]

In D35807#821028, @chandlerc wrote:

I think the command line flags can be made simpler, but I must not be explaining it well in review. I suggest you go ahead and submit this and I'll immediately send you a follow-up patch that hopefully illustrates what I'm suggesting for handling the flags. And that will let you rebase and continue on other things.

Thanks, will do now.

include/llvm/Passes/PassBuilder.h

 
 namespace llvm {
 class StringRef;
 class AAManager;
 class TargetMachine;
 
 /// A struct capturing PGO tunables.
 struct PGOOptions {
-  std::string ProfileGenFile = "";
-  std::string ProfileUseFile = "";
-  std::string SampleProfileFile = "";
-  bool RunProfileGen = false;
+  PGOOptions(std::string ProfileGenFile = "", std::string ProfileUseFile = "",
+             std::string SampleProfileFile = "", bool RunProfileGen = false)
+      : ProfileGenFile(ProfileGenFile), ProfileUseFile(ProfileUseFile),
+        SampleProfileFile(SampleProfileFile), RunProfileGen(RunProfileGen) {}
+  std::string ProfileGenFile;
+  std::string ProfileUseFile;
+  std::string SampleProfileFile;
+  bool RunProfileGen;
 };
 
 /// \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.

lib/Passes/PassBuilder.cpp

 static cl::opt<bool> EnableGVNHoist(
     "enable-npm-gvn-hoist", cl::init(false), cl::Hidden,
     cl::desc("Enable the GVN hoisting pass for the new PM (default = off)"));
 
 static cl::opt<bool> EnableGVNSink(
     "enable-npm-gvn-sink", cl::init(false), cl::Hidden,
     cl::desc("Enable the GVN hoisting pass for the new PM (default = off)"));
 
+static cl::opt<std::string> PGOOptString(
+    "pgo-opt", cl::init(""), cl::Hidden,
+    cl::desc("PGOOpt string in the format of "
+             "{gen|use|sample_gen|sample_use}=value"));
+
 static Regex DefaultAliasRegex(
     "^(default|thinlto-pre-link|thinlto|lto-pre-link|lto)<(O[0123sz])>$");
 
 static bool isOptimizingForSize(PassBuilder::OptimizationLevel Level) {
   switch (Level) {
   case PassBuilder::O0:
   case PassBuilder::O1:
   case PassBuilder::O2:
if (!Callbacks.empty()) {
     PassManagerT DummyPM;
     for (auto &CB : Callbacks)
       if (CB(Name, DummyPM, {}))
         return true;
   }
   return false;
 }
 
+/// Processes user specifiy -pgo-opt option to override PGOOpt.
+static void processPGOOpt(Optional<PGOOptions> &PGOOpt) {
+  if (PGOOptString.empty())
+    return;
+
+  auto pos = PGOOptString.find('=');
+  if (pos == std::string::npos)
+    report_fatal_error("Invalid -pgo-opt.");
+
+  std::string K = PGOOptString.substr(0, pos);
+  std::string V = PGOOptString.substr(pos + 1);
+
+  PGOOpt = StringSwitch<Optional<PGOOptions>>(K)
+               .Case("gen", PGOOptions(V, "", "", true))
+               .Case("use", PGOOptions("", V, "", false))
+               .Case("sample-use", PGOOptions("", "", V, false))
+               .Default(None);
+  if (!PGOOpt)
+    report_fatal_error("Invalid -pgo-opt.");
+}
+
 template <typename CallbacksT>
 static bool isModulePassName(StringRef Name, CallbacksT &Callbacks) {
   // Manually handle aliases for pre-configured pipeline fragments.
   if (startsWithDefaultPipelineAliasPrefix(Name))
     return DefaultAliasRegex.match(Name);
 
   // Explicitly handle pass manager names.
   if (Name == "module")
     return true;
   if (Name == "cgscc")
     return true;
   if (Name == "function")
     return true;
 

 // pre-populate the analysis managers with target-specific stuff?
 bool PassBuilder::parsePassPipeline(ModulePassManager &MPM,
                                     StringRef PipelineText, bool VerifyEachPass,
                                     bool DebugLogging) {
   auto Pipeline = parsePipelineText(PipelineText);
   if (!Pipeline || Pipeline->empty())
     return false;
 
+  processPGOOpt(PGOOpt);
+
   // If the first name isn't at the module layer, wrap the pipeline up
   // automatically.
   StringRef FirstName = Pipeline->front().Name;
 
   if (!isModulePassName(FirstName, ModulePipelineParsingCallbacks)) {
     if (isCGSCCPassName(FirstName, CGSCCPipelineParsingCallbacks)) {
       Pipeline = {{"cgscc", std::move(*Pipeline)}};
     } else if (isFunctionPassName(FirstName,

test/Other/Inputs/new-pm-pgo.prof

+foo:0:0

test/Other/Inputs/new-pm-pgo.proftext

+:ir

test/Other/new-pm-pgo.ll

+; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-opt='gen=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-opt='use=%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE
+; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-opt='sample-use=%S/Inputs/new-pm-pgo.prof' %s 2>&1 |FileCheck %s --check-prefixes=SAMPLE_USE
+;
+; GEN: Running pass: PGOInstrumentationGen
+; USE: Running pass: PGOInstrumentationUse
+; SAMPLE_USE: Running pass: SampleProfileLoaderPass
+
+define void @foo() {
+  ret void
+}