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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
