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 ↗

(On Diff #107927)

s/Use/Process/ ?

1116 ↗

(On Diff #107927)

Do you need return after fatal?

1118 ↗

(On Diff #107927)

Capital P

1120–1135 ↗

(On Diff #107927)

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 ↗

(On Diff #107939)

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 ↗

(On Diff #107939)

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

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.

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-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-use='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE
+; RUN: opt -debug-pass-manager -passes='default<O2>' -pgo-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
+}

tools/opt/NewPMDriver.cpp

static cl::opt<std::string> CGSCCOptimizerLateEPPipeline(
     cl::desc("A textual description of the cgscc pass pipeline inserted at "
              "the CGSCCOptimizerLate extension point into default pipelines"),
     cl::Hidden);
 static cl::opt<std::string> VectorizerStartEPPipeline(
     "passes-ep-vectorizer-start",
     cl::desc("A textual description of the function pass pipeline inserted at "
              "the VectorizerStart extension point into default pipelines"),
     cl::Hidden);
+static cl::opt<std::string> PGOInstrGen(
+    "pgo-gen", cl::desc("Path to the instrumented profile. If set, "
+                        "enable PGO instrumentation."),
+    cl::Hidden);
+static cl::opt<std::string> PGOInstrUse(
+    "pgo-use", cl::desc("Path to the instrumented profile. If set, "
+                        "enable PGO optimization."),
+    cl::Hidden);
+static cl::opt<std::string> PGOSampleUse(
+    "pgo-sample-use", cl::desc("Path to the sample profile. If set, enable "
+                               "SamplePGO optimization."),
+    cl::Hidden);
 /// @}}
 
 template <typename PassManagerT>
 bool tryParsePipelineText(PassBuilder &PB, StringRef PipelineText) {
   if (PipelineText.empty())
     return false;
 
   // Verify the pipeline is parseable:
bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                            tool_output_file *Out,
                            tool_output_file *ThinLTOLinkOut,
                            StringRef PassPipeline, OutputKind OK,
                            VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder,
                            bool EmitSummaryIndex, bool EmitModuleHash) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
-  PassBuilder PB(TM);
+  int NumPGOOptions = (PGOInstrGen.empty() ? 0 : 1) +
+                      (PGOInstrUse.empty() ? 0 : 1) +
+                      (PGOSampleUse.empty() ? 0 : 1);
+  assert(NumPGOOptions <2 &&
+         "Only one of -pgo-gen,-pgo-use,-pgo-sample-use can be "
+         "specified");
+  Optional<PGOOptions> P;
+  if (NumPGOOptions)
+    P = PGOOptions(PGOInstrGen, PGOInstrUse, PGOSampleUse,
+                   !PGOInstrGen.empty());
+  PassBuilder PB(TM, P);
   registerEPCallbacks(PB, VerifyEachPass, DebugPM);
 
   // Specially handle the alias analysis manager so that we can register
   // a custom pipeline of AA passes with it.
   AAManager AA;
   if (!PB.parseAAPipeline(AA, AAPipeline)) {
     errs() << Arg0 << ": unable to parse AA pipeline description.\n";
     return false;