Refine the PGOOpt and SamplePGOSupport handling.

[danielcdh]

Summary

Now that SamplePGOSupport is part of PGOOpt, there are several places that need tweaking:

1. AddDiscriminator pass should *not* be invoked at ThinLTOBackend (as it's already invoked in the PreLink phase)
2. addPGOInstrPasses should only be invoked when either ProfileGenFile or ProfileUseFile is non-empty.
3. SampleProfileLoaderPass should only be invoked when SampleProfileFile is non-empty.
4. PGOIndirectCallPromotion should only be invoked in ProfileUse phase, or in ThinLTOBackend of SamplePGO.

[chandlerc]

I think this adds way too much complexity to these predicates. I think we need a much better approach here... Have you looked at alternatives? Is the technique being used to add the descriminators pass just the wrong technique given the interaction with ThinLTO

[danielcdh]

In D36040#825041, @chandlerc wrote:

I think this adds way too much complexity to these predicates. I think we need a much better approach here... Have you looked at alternatives? Is the technique being used to add the descriminators pass just the wrong technique given the interaction with ThinLTO

This was how we handle AddDiscriminator pass in the legacy PM. I have not come up with a better alternative, could you help suggest? Thanks

[chandlerc]

Mostly see how to improve ThinLTO flags here. Need to understand better the other changes.

lib/Passes/PassBuilder.cpp

537–538

I'd suggest hoisting this into the caller, and its own module-to-function adaptor so we walk the entire module and add discriminators once befare we do anything else. That'll match the other similar "semantic" pass we have where we force certain function attributes.

574–576

I don't think this assert is adding much value and it makes things harder to read...

I would move this assert into maybe more precise asserts in the PGOOpt class itself.

Then I think a comment here explaining the logic would help. Before, it seemed obvious: if we're not doing sample profiling, add the instrumentation-based profiling passes. Otherwise, add the sample passes. Now the logic seems more complex....

586–588

Why do we want to do this *before* the thin link with instrumentation PGO but not with sample PGO?

[danielcdh]

lib/Passes/PassBuilder.cpp

574–576

Moved the assert to PGOOptions class.

Added the comment to make it more clear. I think the logic is still the same, the first if handles instrumentation based PGO, the 2nd handles sample based PGO.

586–588

As illustrated in the comment, we do not enable it in PrepareForThinLTO phase during sample PGO because it changes IR to makes profile annotation in backend compile inaccurate.

I'm not sure why ICP is invoked twice in both prelink and backend.

[danielcdh]

Integrate Chandler's reviews. Thanks!

[chandlerc]

lib/Passes/PassBuilder.cpp

574–576

So, the logic hasn't actually changed? If not, maybe leave the original code? I found it a bit easier to read. Not a huge deal either way.

782–785

You don't actually need an FPM, you can directly hand the pass to the adaptor.

[danielcdh]

update

[danielcdh]

lib/Passes/PassBuilder.cpp

574–576

Sorry, did not make this clear: the old logic was trying to do the same thing (handle instrPGO and samplePGO in the branch). But the original logic was wrong. It assumes that if SampleProfileFile is empty, then one of the InstrProfile flags will be non-empty. This assumption was broken when we introduced SamplePGOSupport. As all 3 file paths could be empty, while SamplePGOSupport==true.

[chandlerc]

LGTM w/ tweak below.

lib/Passes/PassBuilder.cpp

586–588

I suspect we should follow-up on this and change sample PGO and instrumentation PGO to behave the same way here (or have a clear explanation of why they differ).

But that can happen later.

805–808

Same as above, can directly add the pass to the adaptor.

[danielcdh]

lib/Passes/PassBuilder.cpp

586–588

Acknowledged.

Teresa and David, do you remember why we invoke ICP twice in instrumentation based FDO?

[danielcdh]

update

include/llvm/Passes/PassBuilder.h

 
 /// A struct capturing PGO tunables.
 struct PGOOptions {
   PGOOptions(std::string ProfileGenFile = "", std::string ProfileUseFile = "",
              std::string SampleProfileFile = "", bool RunProfileGen = false,
              bool SamplePGOSupport = false)
       : ProfileGenFile(ProfileGenFile), ProfileUseFile(ProfileUseFile),
         SampleProfileFile(SampleProfileFile), RunProfileGen(RunProfileGen),
-        SamplePGOSupport(SamplePGOSupport || !SampleProfileFile.empty()) {}
+        SamplePGOSupport(SamplePGOSupport || !SampleProfileFile.empty()) {
+    assert((RunProfileGen ||
+            !SampleProfileFile.empty() ||
+            !PGOOpt->ProfileUseFile.empty() ||
+            PGOOpt->SamplePGOSupport) && "Illegal PGOOptions.");
+  }
   std::string ProfileGenFile;
   std::string ProfileUseFile;
   std::string SampleProfileFile;
   bool RunProfileGen;
   bool SamplePGOSupport;
 };
 
 /// \brief This class provides access to building LLVM's passes.

lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   ModulePassManager MPM(DebugLogging);
 
   // 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.
   // FIXME: This position in the pipeline hasn't been carefully considered in
PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   FunctionPassManager GlobalCleanupPM(DebugLogging);
   GlobalCleanupPM.addPass(InstCombinePass());
   invokePeepholeEPCallbacks(GlobalCleanupPM, Level);
 
   GlobalCleanupPM.addPass(SimplifyCFGPass());
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(GlobalCleanupPM)));
 
   // Add all the requested passes for PGO, if requested.
   if (PGOOpt) {
-    assert(PGOOpt->RunProfileGen || !PGOOpt->SampleProfileFile.empty() ||
-           !PGOOpt->ProfileUseFile.empty() || PGOOpt->SamplePGOSupport);
-    if (PGOOpt->SampleProfileFile.empty())
+    if (!PGOOpt->ProfileGenFile.empty() || !PGOOpt->ProfileUseFile.empty())
+      // Instrumentation based PGO (gen and use)
       addPGOInstrPasses(MPM, DebugLogging, Level, PGOOpt->RunProfileGen,
                         PGOOpt->ProfileGenFile, PGOOpt->ProfileUseFile);
-    else
+    else if (!PGOOpt->SampleProfileFile.empty())
+      // SamplePGO use
       MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
 
     // Indirect call promotion that promotes intra-module targes only.
     // Do not enable it in PrepareForThinLTO phase during sample PGO because
     // it changes IR to makes profile annotation in back compile inaccurate.
-    if (!PrepareForThinLTO || PGOOpt->SampleProfileFile.empty())
+    if ((!PrepareForThinLTO && !PGOOpt->SampleProfileFile.empty())
+        || !PGOOpt->ProfileUseFile.empty())
       MPM.addPass(PGOIndirectCallPromotion(
           false, PGOOpt && !PGOOpt->SampleProfileFile.empty()));
   }
 
   // Require the GlobalsAA analysis for the module so we can query it within
   // the CGSCC pipeline.
   MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
 
   // Require the ProfileSummaryAnalysis for the module so we can query it within
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 to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
+  if (PGOOpt && PGOOpt->SamplePGOSupport)
+    MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
+
   // Add the core simplification pipeline.
   MPM.addPass(buildModuleSimplificationPipeline(Level, DebugLogging,
                                                 /*PrepareForThinLTO=*/false));
 
   // Now add the optimization pipeline.
   MPM.addPass(buildModuleOptimizationPipeline(Level, DebugLogging));
 
   return MPM;
 }
 
 ModulePassManager
 PassBuilder::buildThinLTOPreLinkDefaultPipeline(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 to observe.
   MPM.addPass(ForceFunctionAttrsPass());
 
+  if (PGOOpt && PGOOpt->SamplePGOSupport)
+    MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
+
   // If we are planning to perform ThinLTO later, we don't bloat the code with
   // unrolling/vectorization/... now. Just simplify the module as much as we
   // can.
   MPM.addPass(buildModuleSimplificationPipeline(Level, DebugLogging,
                                                 /*PrepareForThinLTO=*/true));
 
   // Run partial inlining pass to partially inline functions that have
   // large bodies.
   // FIXME: It isn't clear whether this is really the right place to run this
   // in ThinLTO. Because there is another canonicalization and simplification

test/Other/new-pm-thinlto-defaults.ll

 ; The IR below was crafted so as:
 ; 1) To have a loop, so we create a loop pass manager
 ; 2) To be "immutable" in the sense that no pass in the standard
 ;    pipeline will modify it.
 ; Since no transformations take place, we don't expect any analyses
 ; to be invalidated.
 ; Any invalidation that shows up here is a bug, unless we started modifying
 ; the IR, in which case we need to make it immutable harder.
 ;
 ; Prelink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto-pre-link<O1>,name-anon-globals' -S %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-PRELINK-O,CHECK-PRELINK-O1
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-PRELINK-O,CHECK-PRELINK-O-NODIS,CHECK-PRELINK-O1
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto-pre-link<O2>,name-anon-globals' -S  %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-PRELINK-O,CHECK-PRELINK-O2
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-PRELINK-O,CHECK-PRELINK-O-NODIS,CHECK-PRELINK-O2
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto-pre-link<O3>,name-anon-globals' -S  %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-PRELINK-O,CHECK-PRELINK-O3
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-PRELINK-O,CHECK-PRELINK-O-NODIS,CHECK-PRELINK-O3
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto-pre-link<Os>,name-anon-globals' -S %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Os,CHECK-PRELINK-O,CHECK-PRELINK-Os
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Os,CHECK-PRELINK-O,CHECK-PRELINK-O-NODIS,CHECK-PRELINK-Os
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto-pre-link<Oz>,name-anon-globals' -S %s 2>&1 \
-; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-PRELINK-O,CHECK-PRELINK-Oz
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-PRELINK-O,CHECK-PRELINK-O-NODIS,CHECK-PRELINK-Oz
+; RUN: opt -disable-verify -debug-pass-manager -new-pm-debug-info-for-profiling \
+; RUN:     -passes='thinlto-pre-link<O2>,name-anon-globals' -S  %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefixes=CHECK-DIS,CHECK-O,CHECK-O2,CHECK-PRELINK-O,CHECK-PRELINK-O2
 ;
 ; Postlink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<O1>' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-POSTLINK-O,CHECK-POSTLINK-O1
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<O2>' -S  %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-POSTLINK-O,CHECK-POSTLINK-O2
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<O3>' -S  %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-POSTLINK-O,CHECK-POSTLINK-O3
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<Os>' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Os,CHECK-POSTLINK-O,CHECK-POSTLINK-Os
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<Oz>' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-POSTLINK-O,CHECK-POSTLINK-Oz
+; RUN: opt -disable-verify -debug-pass-manager -new-pm-debug-info-for-profiling \
+; RUN:     -passes='thinlto<O2>' -S  %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-POSTLINK-O,CHECK-POSTLINK-O2
 ;
 ; CHECK-O: Starting llvm::Module pass manager run.
 ; CHECK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Module pass manager run.
 ; CHECK-O-NEXT: Running pass: ForceFunctionAttrsPass
+; CHECK-DIS-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}Function>
+; CHECK-DIS-NEXT: Running analysis: InnerAnalysisManagerProxy
+; CHECK-DIS-NEXT: Starting llvm::Function pass manager run.
+; CHECK-DIS-NEXT: Running pass: AddDiscriminatorsPass
+; CHECK-DIS-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-POSTLINK-O-NEXT: Running pass: PGOIndirectCallPromotion
 ; CHECK-POSTLINK-O-NEXT: Running analysis: InnerAnalysisManagerProxy<{{.*}}Function
 ; CHECK-POSTLINK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Module pass manager run.
 ; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
-; CHECK-PRELINK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
+; CHECK-PRELINK-O-NODIS-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
 ; CHECK-O-NEXT: Running pass: SROA
 ; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis