[PATCH] D36040: Refine the PGOOpt and SamplePGOSupport handling.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM w/ tweak below.



================
Comment at: lib/Passes/PassBuilder.cpp:589-590
     // 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(
----------------
danielcdh wrote:
> chandlerc wrote:
> > Why do we want to do this *before* the thin link with instrumentation PGO but not with sample PGO?
> 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.
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.


================
Comment at: lib/Passes/PassBuilder.cpp:805-808
+    FunctionPassManager AddDiscriminatorFPM(DebugLogging);
+    AddDiscriminatorFPM.addPass(AddDiscriminatorsPass());
+    MPM.addPass(
+        createModuleToFunctionPassAdaptor(std::move(AddDiscriminatorFPM)));
----------------
Same as above, can directly add the pass to the adaptor.


https://reviews.llvm.org/D36040