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

[Dehao Chen via Phabricator via llvm-commits]

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:
> 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.
Sorry, did not make this clear: the old logic was trying to do the same thing (handle instrPGO and samplePGO in the branch). But the original logic was wrong. It assumes that if SampleProfileFile is empty, then one of the InstrProfile flags will be non-empty. This assumption was broken when we introduced SamplePGOSupport. As all 3 file paths could be empty, while SamplePGOSupport==true.


https://reviews.llvm.org/D36040