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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh marked an inline comment as done.
danielcdh added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:578-580
     assert(PGOOpt->RunProfileGen || !PGOOpt->SampleProfileFile.empty() ||
            !PGOOpt->ProfileUseFile.empty() || PGOOpt->SamplePGOSupport);
+    if (!PGOOpt->ProfileGenFile.empty() || !PGOOpt->ProfileUseFile.empty())
----------------
chandlerc wrote:
> 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....
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.


================
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(
----------------
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.


https://reviews.llvm.org/D36040