[PATCH] D36333: Move the SampleProfileLoader right after EarlyFPM.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:548
+      Phase == ThinLTOPhase::PostLink)
+    EarlyFPM.addPass(InstCombinePass());
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(EarlyFPM)));
----------------
tejohnson wrote:
> What is the downside of always doing InstCombine here?
Increase compile time?

Additionally, I'd like to make this patch NFC for other compilation models.


================
Comment at: lib/Passes/PassBuilder.cpp:552
+  if (PGOOpt && !PGOOpt->SampleProfileFile.empty()) {
+    // Annotate sample profile right after early FPM to ensure freshness of
+    // the debug info.
----------------
tejohnson wrote:
> Also needs comment about why before globalopt.
comments added, also added test to cover that.


================
Comment at: lib/Passes/PassBuilder.cpp:851
   // look unreferenced and are removed.
-  MPM.addPass(PGOIndirectCallPromotion(
-      true /* InLTO */, PGOOpt && !PGOOpt->SampleProfileFile.empty()));
+  if (PGOOpt && !PGOOpt->ProfileUseFile.empty())
+    MPM.addPass(PGOIndirectCallPromotion(true /* InLTO */,
----------------
tejohnson wrote:
> Can we simply invoke PGO ICP always from within buildModuleSimplificationPipeline if we're not in the ThinLTO PreLink phase? I.e. regardless of Sample or Instrumented PGO? It seems cleaner to do it in the same place if possible.
Yes, but I'd like to make this patch NFC for other modes such as instrumentation based PGO. Added FIXME


https://reviews.llvm.org/D36333