[PATCH] D27324: IPO: Introduce ThinLTOBitcodeWriter pass.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

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.

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

> Also avoids redundancy in the ThinLTO handling.

It's not a lot of code to be honest, most of the implementation is in BitcodeWriter which is already shared.



================
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,
----------------
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).


https://reviews.llvm.org/D27324