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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

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



================
Comment at: lib/Passes/PassBuilder.cpp:539-540
   FunctionPassManager EarlyFPM(DebugLogging);
-  if (PGOOpt && PGOOpt->SamplePGOSupport)
+  if (PGOOpt && PGOOpt->SamplePGOSupport && !ThinLTOBackend)
     EarlyFPM.addPass(AddDiscriminatorsPass());
   EarlyFPM.addPass(SimplifyCFGPass());
----------------
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.


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


================
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(
----------------
Why do we want to do this *before* the thin link with instrumentation PGO but not with sample PGO?


https://reviews.llvm.org/D36040