[PATCH] D36052: Update the new PM pipeline to make ICP aware if it is SamplePGO build.
Dehao Chen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jul 29 18:22:48 PDT 2017
danielcdh added a comment.
In https://reviews.llvm.org/D36052#825325, @chandlerc wrote:
> In https://reviews.llvm.org/D36052#825321, @danielcdh wrote:
> > In https://reviews.llvm.org/D36052#825318, @davidxl wrote:
> > > Instr PGO has its own set of simplification passes, but still I think we should keep this patch NFC for instrPGO. If there are performance numbers that justify the InstrPGO pipeline changes, it can be discussed/done in a separate thread. This one should be left as SamplePGO only change.
> FWIW, I do agree with you Dehao that we seem to be deing redundant simplification, but as David says here we shouldn't change that behavior in this patch.
> > The tricky part is that PGOOptions was not passed in the ThinLTO backend before this patch. But after this patch, it will be passed in. However, in buildModuleSimplificationPipeline, it cannot know if this is the ThinLTO backend. As a result, we cannot disable instrumentation/annotation pass in there.
> > The potential solution could be:
> > - add another boolean value in buildModuleSimplificationPipeline to indicate it's ThinLTO backend
> > - refactor the code from the beginning of buildModuleSimplificationPipeline to the PGOOpt handling to a separate function, and call it separately in the caller of buildModuleSimplificationPipeline.
> I actually think the second option might be interesting, but before we do that I think understanding how much redundancy there really is in the cleanup passes is important. That will help inform how to refactor these into separate components, or if there even *are* separate components. So I largely agree w/ David that something like #1 is the best path forward.
> However, rather than adding another boolean value though, I think you should turn the current boolean into an enumeration for ThinLTO phase; "none" by default, and then with beth prelink and postlink values.
> Note that I don't think we should call the phase "backend" or "codegen". Those are heavily overloaded in LLVM, so I think pre/post-link is a better terminology to use.
Thanks Chandler and David for the suggestion.
I've sent a separate patch https://reviews.llvm.org/D36053 to refactor the code. The patch is NFC, but I'd like to ask for your opinions on the naming/description to see if it's concise. Thanks in advance for comments.
More information about the llvm-commits