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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc 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())
----------------
danielcdh wrote:
> 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.
So, the logic hasn't actually changed? If not, maybe leave the original code? I found it a bit easier to read. Not a huge deal either way.


================
Comment at: lib/Passes/PassBuilder.cpp:782-785
+    FunctionPassManager AddDiscriminatorFPM(DebugLogging);
+    AddDiscriminatorFPM.addPass(AddDiscriminatorsPass());
+    MPM.addPass(
+        createModuleToFunctionPassAdaptor(std::move(AddDiscriminatorFPM)));
----------------
You don't actually need an FPM, you can directly hand the pass to the adaptor.


https://reviews.llvm.org/D36040