Make new PM honor -fdebug-info-for-profiling

[danielcdh]

Summary

The new PM needs to invoke add-discriminator pass when building with -fdebug-info-for-profiling.

[danielcdh]

Chandler, could you help suggest how I could add unittest for this change? I was trying to find similar test for PGOOpt, but could not find any tests that's related.

Thanks,
Dehao

[chandlerc]

include/llvm/Passes/PassBuilder.h

54

(If we end up using this flag, see below) Maybe call this "AddDebugInfoDiscriminators"? or otherwise make it more clear what changes it introduces?

lib/Passes/PassBuilder.cpp

333–334

Does this need to be at this *exact* position? Could it be done before all passes and directly added to the pipeline by the frontend?

Alternatively, could you use one of the extension points we have in the new PM for this?

If not, I would suggest that we need an opt flag that sets this and test it that way.

Yet another alternative: what about keying this off of the existing PGO options rather than a separate flag? (Side note: do we have a way to set up the PGO options using the opt tool?)

[danielcdh]

lib/Passes/PassBuilder.cpp

333–334

This does not need to be at the exact position. It should be done before all passes. I'm not sure how to directly add this to the pipeline by FE, looks like all the passes added by FE are all module passes. As AddDiscriminatorsPass is a function pass, I'm not sure how to add it to the MPM. Could you suggest how to do that in new PM?

Could you point me to an example of how to use extension points in the new PM?

We could add this to PGOOpt if you think that is a good option. I don't think that we could pass in the PGOOpt to the opt tool.

Thanks,
Dehao

[chandlerc]

lib/Passes/PassBuilder.cpp

333–334

The more I think about it, the more I think adding this pass should be triggered much like the other profiling-specific passes are triggered from PGOOpt.

And I really do think we want a way to setup and pass in PGOOpt from the opt commandline. What seems challenging there?

[danielcdh]

update the option to put it within PGOOPtions. Will add test once https://reviews.llvm.org/D35807 is committed.

[chandlerc]

include/llvm/Passes/PassBuilder.h

32

Calling this profile generation seems odd.

I would call this more along the lines of supporting sample PGO *at all* (even if not using a specific profile file), and then set it to true if a file is provided (which will become more obvious with a constructor when rebased on your other patch).

Then the condition below seems simpler.

[danielcdh]

include/llvm/Passes/PassBuilder.h

32

Updated the patch to change the name, also added tests.

Not sure what name to use at the opt driver, any suggestions?

Thanks,
Dehao

[danielcdh]

update flag name and add test.

[chandlerc]

tools/opt/NewPMDriver.cpp

95–97

Generating more PGO-friendly debug information seems orthogonal to the others (but perhaps reasonable to default "on" when *using* a sample profile), so I would leave it as a separate flag entirely.

I don't have great naming ideas... I guess steal from the Clang name: -new-pm-debug-info-for-profiling.

[danielcdh]

update

[chandlerc]

LGTM

[modocache]

Just FYI, test/Other/new-pm-pgo.ll appears to be failing on several of the buildbots. One example: http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/10801

[danielcdh]

In D35744#821589, @modocache wrote:

Just FYI, test/Other/new-pm-pgo.ll appears to be failing on several of the buildbots. One example: http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/10801

Thanks for the heads up.

I only tested this on release compiler, thus assertion failure is not triggered. Sorry about that.

Committed r309125 as NFC to fix the issue. Please let me know if you think that I should revert both changes and send a new one for review.

include/llvm/Passes/PassBuilder.h

 
 namespace llvm {
 class StringRef;
 class AAManager;
 class TargetMachine;
 
 /// A struct capturing PGO tunables.
 struct PGOOptions {
   PGOOptions(std::string ProfileGenFile = "", std::string ProfileUseFile = "",
-             std::string SampleProfileFile = "", bool RunProfileGen = false)
+             std::string SampleProfileFile = "", bool RunProfileGen = false,
+             bool SamplePGOSupport = false)
       : ProfileGenFile(ProfileGenFile), ProfileUseFile(ProfileUseFile),
-        SampleProfileFile(SampleProfileFile), RunProfileGen(RunProfileGen) {}
+        SampleProfileFile(SampleProfileFile), RunProfileGen(RunProfileGen),
+        SamplePGOSupport(SamplePGOSupport || !SampleProfileFile.empty()) {}
   std::string ProfileGenFile;
   std::string ProfileUseFile;
   std::string SampleProfileFile;
   bool RunProfileGen;
+  bool SamplePGOSupport;
 };
 
 /// \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 A struct to capture parsed pass pipeline names.
   ///
   /// A pipeline is defined as a series of names, each of which may in itself
   /// recursively contain a nested pipeline. A name is either the name of a pass
   /// (e.g. "instcombine") or the name of a pipeline type (e.g. "cgscc"). If the
   /// name is the name of a pass, the InnerPipeline is empty, since passes
   /// cannot contain inner pipelines. See parsePassPipeline() for a more

lib/Passes/PassBuilder.cpp

 
 FunctionPassManager
 PassBuilder::buildFunctionSimplificationPipeline(OptimizationLevel Level,
                                                  bool DebugLogging,
                                                  bool PrepareForThinLTO) {
   assert(Level != O0 && "Must request optimizations!");
   FunctionPassManager FPM(DebugLogging);
 
   // Form SSA out of local memory accesses after breaking apart aggregates into
   // scalars.
   FPM.addPass(SROA());
 
   // Catch trivial redundancies
   FPM.addPass(EarlyCSEPass(EnableEarlyCSEMemSSA));
 
   // Hoisting of scalars and load expressions.
   if (EnableGVNHoist)
     FPM.addPass(GVNHoistPass());
PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
 
   // Do basic inference of function attributes from known properties of system
   // libraries and other oracles.
   MPM.addPass(InferFunctionAttrsPass());
 
   // Create an early function pass manager to cleanup the output of the
   // frontend.
   FunctionPassManager EarlyFPM(DebugLogging);
+  if (PGOOpt && PGOOpt->SamplePGOSupport)
+    EarlyFPM.addPass(AddDiscriminatorsPass());
   EarlyFPM.addPass(SimplifyCFGPass());
   EarlyFPM.addPass(SROA());
   EarlyFPM.addPass(EarlyCSEPass());
   EarlyFPM.addPass(LowerExpectIntrinsicPass());
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(EarlyFPM)));
 
   // Interprocedural constant propagation now that basic cleanup has occured
   // and prior to optimizing globals.

test/Other/new-pm-pgo.ll

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

tools/opt/NewPMDriver.cpp

static cl::opt<PGOKind> PGOKindFlag(
     cl::desc("The kind of profile guided optimization"),
     cl::values(clEnumValN(NoPGO, "nopgo", "Do not use PGO."),
                clEnumValN(InstrGen, "new-pm-pgo-instr-gen-pipeline",
                           "Instrument the IR to generate profile."),
                clEnumValN(InstrUse, "new-pm-pgo-instr-use-pipeline",
                           "Use instrumented profile to guide PGO."),
                clEnumValN(SampleUse, "new-pm-pgo-sample-use-pipeline",
                           "Use sampled profile to guide PGO.")));
 static cl::opt<std::string> ProfileFile(
     "profile-file", cl::desc("Path to the profile."), cl::Hidden);
+static cl::opt<bool> DebugInfoForProfiling(
+    "new-pm-debug-info-for-profiling", cl::init(false), cl::Hidden,
+    cl::desc("Emit special debug info to enable PGO profile generation."));
 /// @}}
 
 template <typename PassManagerT>
 bool tryParsePipelineText(PassBuilder &PB, StringRef PipelineText) {
   if (PipelineText.empty())
     return false;
 
   // Verify the pipeline is parseable:
case InstrGen:
       break;
     case InstrUse:
       P = PGOOptions("", ProfileFile, "", false);
       break;
     case SampleUse:
       P = PGOOptions("", "", ProfileFile, false);
       break;
     case NoPGO:
-      P = None;      
+      if (DebugInfoForProfiling)
+        P = PGOOptions("", "", "", false, true);
+      else
+        P = None;
   }
   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)) {