[PATCH] D36052: Update the new PM pipeline to make ICP aware if it is SamplePGO build.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:765-767
+    if (!PGOOpt->ProfileGenFile.empty() || !PGOOpt->ProfileUseFile.empty())
+      addPGOInstrPasses(MPM, DebugLogging, Level, PGOOpt->RunProfileGen,
+                        PGOOpt->ProfileGenFile, PGOOpt->ProfileUseFile);
----------------
chandlerc wrote:
> This is a *very* significant change in the PGO instrumentation pass ordering.
> 
> Before, we would do non-trivial cleanup of the IR module before adding instrumentation passes. Now they get added before any of that cleanup. This includes thing such as the first (and primary) SSA formation pass, lowering @llvm.expect intrinsics into branch weight metadata, dead argument elimination, global-opt, etc.
> 
> Was all of that intended? Is it really OK to instrument before those cleanups?
There are still some optimizations invoked in addPGOInstrPasses before the actual instrumentation:

    FunctionPassManager FPM;
    FPM.addPass(SROA());
    FPM.addPass(EarlyCSEPass());    // Catch trivial redundancies.
    FPM.addPass(SimplifyCFGPass()); // Merge & remove basic blocks.
    FPM.addPass(InstCombinePass()); // Combine silly sequences.
    invokePeepholeEPCallbacks(FPM, Level);

    CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));

    MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));

If we invoke this in the original location, SROA/EarlyCSE/SimplifyCFG will be invoked twice before entering PGO instr/use pass, seems redundant to me. I've also tested the instrumentation PGO on our largest benchmark, and did not see performance change.

I know these EarlyFPM were to match the behavior of the legacy PM. But maybe it should not belong to buildModuleSimplificationPipeline, which is called by both regular pipeline, prelink, and thinlto default pipeline. At least thinlto default pipeline does not need these passes?


https://reviews.llvm.org/D36052