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

Chandler Carruth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 28 18:48:32 PDT 2017

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.


More information about the llvm-commits mailing list