[PATCH] D57265: [PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting

Vedant Kumar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 15:38:08 PDT 2019


vsk added a comment.

In D57265#1393814 <https://reviews.llvm.org/D57265#1393814>, @fedor.sergeev wrote:

> In D57265#1393453 <https://reviews.llvm.org/D57265#1393453>, @vsk wrote:
>
> > > I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.
> >
> > I'm not sure I understand the distinction being made here -- could you elaborate? Isn't enabling a pass related to pipeline-building in general? If not, I don't see how arguments to build*Pipeline //do// become related to pipeline-building in general.
> >
> > For context, I've modeled the addition of the SplitColdCode member to PassBuilder on PGOOpt (c.f. PGOOptions::RunProfileGen). One thing I like about this approach is that it doesn't require changing IR, or changing very many different APIs. But if it's really not reasonable, I'm happy to take another shot at it.
>
>
> I cant say for PGOOpt in particular, but overall idea of PassBuilder is that, well, it is not "builder" as per "builder pattern".
>  You do not fill it with parts of a complex pipeline object and then press a magical "build" button.
>  Instead you ask it to build various "named" pipelines, or just parse it from text.
>  If you compare with PassManagerBuilder, which contains a dozen of various data members that seem to drive the pipeline construction,
>  PassBuilder contains only a few. And the purpose of PassBuilder members is to be used during individual *pass*es creation.
>
> Say, you wont find OptLevel there. Instead it is being passed as an argument to build*Pipeline functions.
>
> As for PGOOpts - it seems to be the only member that stands out.
>  And to me it looks like we should remove it from PassBuilder either, and start passing it to build*Pipeline functions as well.
>  But honestly, this is the first time I really looked through the PGOOpts code, so I might be well mistaken.


Thanks everyone for your patient feedback. I think I understand the advantages of keeping state out of the pipeline builder now.

Teresa's earlier suggestion to use an IR attribute seems like a promising approach. In particular, it makes it simple to correctly enable/disable splitting during LTO. I'll try to post an updated patch soon.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57265/new/

https://reviews.llvm.org/D57265





More information about the cfe-commits mailing list