[PATCH] D27324: IPO: Introduce ThinLTOBitcodeWriter pass.

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 2 12:04:36 PST 2016


tejohnson added a comment.

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.

> 
> 
>> 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.



================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:201
+  // FIXME: Try to re-use BSI and PFI from the original module here.
+  ModuleSummaryIndex Index = buildModuleSummaryIndex(*ThinM, nullptr, nullptr);
+  W.writeModule(ThinM.get(), /*ShouldPreserveUseListOrder=*/false, &Index,
----------------
pcc wrote:
> tejohnson wrote:
> > The WriteBitcodePass instead uses the ModuleSummaryIndexAnalysis pass to construct the index. This pass should be consistent with that.
> Note that this is a separate module from the "main" module. We can't just use the main module summary from the pass manager because we may have renamed some of the globals to export them. Running a separate pass manager on the newly created module to build the summary also seems like the wrong approach, as I mentioned in the FIXME I think we'll eventually want to pull profiling information out of the main module analysis and match it up with the globals in ThinM. That would require going outside the bounds of what the pass manager supports (or needs to support, I'd argue).
Ok, that makes sense.


https://reviews.llvm.org/D27324





More information about the llvm-commits mailing list