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

47

(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

35

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

35

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 ↗

(On Diff #108212)

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

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

lib/Passes/PassBuilder.cpp

 
 FunctionPassManager
 PassBuilder::buildFunctionSimplificationPipeline(OptimizationLevel Level,
                                                  bool DebugLogging,
                                                  bool PrepareForThinLTO) {
   assert(Level != O0 && "Must request optimizations!");
   FunctionPassManager FPM(DebugLogging);
 
+  if (DebugInfoForProfiling)
+    FPM.addPass(AddDiscriminatorsPass());
+
   // 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.