[PATCH] D27324: IPO: Introduce ThinLTOBitcodeWriter pass.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D27324#612112, @tejohnson wrote:

> In https://reviews.llvm.org/D27324#611987, @pcc wrote:
>
> > In https://reviews.llvm.org/D27324#611918, @tejohnson wrote:
> >
> > > Just skimmed the code so far, and have a high-level question. Why a new pass instead of building this into the existing WriteBitcodePass as an option?
> >
> >
> > I see the WriteBitcodePass as a "pure" serialization pass, whereas this pass will do whatever is required to write out the module for ThinLTO. I also see this pass as potentially  diverging a lot from WriteBitcodePass, e.g. it could be doing code generation for non-importable functions (as I think we discussed a long time ago), which is way outside the bounds of what WriteBitcodePass is or should be doing.
> >
> > > It's confusing because that pass also has the ability to write bitcode for ThinLTO.
> >
> > To a certain extent that's coincidental to the pass's ability to write a summary. I suspect that we'll just want to use that capability in "opt" for testing purposes and use this new pass in production pipelines.
>
>
> Why would we want to write ThinLTO bitcode one way in opt and another way in the production pipelines? I think we should keep it simple and have one way to write the ThinLTO bitcode.


The idea was to allow testing something about the summary writer in isolation without worrying about the rest of the pipeline.

>>> Is the intention to use this new pass instead of WriteBitcodePass when we are building with -flto=thin (i.e. from clang)?
>> 
>> Yes.
>> 
>>> Seems cleaner to use a single pass and not have clang worry about which to invoke.
>> 
>> In the end some code does need to make some choice about which code path to take. The question is whether we want separate passes or one pass that does everything and takes a ton of flags. It may come down to personal preference but at least I think that avoiding flags where possible makes the code more readable.
> 
> But I assume the intention is to always do this under -flto=thin, in which case there's just one parameter, analogous to the existing BitcodeWriterPass's EmitSummaryIndex. I have less of a strong feeling about having this as a separate pass though if we can (eventually) remove the existing ThinLTO handling from BitcodeWriterPass. But I would imagine the splitting should stay optional for the time being.

The idea is that splitting would only happen if you have enabled a feature that needs it (that's what requiresSplit is testing for), so there should be no functional change for existing users once we switch clang to using ThinLTOBitcodeWriterPass.

I'm not sure if this is a good idea yet, but maybe there should just be two bitcode writer passes: LTOBitcodeWriterPass and ThinLTOBitcodeWriterPass. Neither would take flags, and basically they would be the only supported "production" passes for writing LTO bitcode. If something like "opt" wants to go under the hood and test a specific feature of the bitcode writer (e.g. the summary writer), it can run a pass pipeline without a bitcode writer and call the writer itself.


https://reviews.llvm.org/D27324