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

[Fedor Sergeev via Phabricator via cfe-commits]

fedor.sergeev added a comment.

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.


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

https://reviews.llvm.org/D57265