[PATCH] D35334: ThinLTO Minimized Bitcode File Size Reduction

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:4007
+
+/// Class to manage the bitcode writing for a module summary.
+class SummaryBitcodeWriter : public ModuleBitcodeWriterBase {
----------------
mehdi_amini wrote:
> haojiewang wrote:
> > mehdi_amini wrote:
> > > Is a "module summary" described anywhere?
> > I got that name from ModuleSummaryIndex, but it's not a good name. Already changed it to minimized bitcode file.
> I have the same question for "minimized bitcode file" though, it may be some doc debt if this is already used in some place and not defined, just trying to flag these, not blocking your patch.
The FE option is -fthin-link-bitcode, so perhaps ThinLinkBitcodeWriter? Admittedly I need to add documentation for this option.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:4031
+
+  writeModuleInfo();
+
----------------
pcc wrote:
> haojiewang wrote:
> > pcc wrote:
> > > haojiewang wrote:
> > > > pcc wrote:
> > > > > With this call you're emitting a lot of unnecessary information into the minimized bitcode file. As I was suggesting on the other thread I wonder whether you can just emit "short" module-level records that just contain the name and the linkage (maybe see if you can drop the linkage, as it should already be available in the summary).
> > > > Yes, this is what I'm going to do after this patch released. So do you suggest I do it in this patch or leave it to next patch?
> > > I would suggest writing an initial patch that changes the index bitcode reader to read the linkage from the summary instead of from the module, and then once that lands do what I suggested in this patch.
> > If it is ok that we do that after this patch? Because I think module info have too much redundancy info for minimized bitcode file, most of which is necessary for IR bitcode file. We can add a new writeModuleInfo for minimized bitcode file after this patch, and then also change the bitcode reader. That can get 5%-10% benefit on size reduction.
> Up to you but keep in mind that your code will be easier to review if you do things in the order I suggested, because we won't have to think about the design that we know you're going to replace, just the one that you're going to replace it with.
That change seems like an incremental follow on to this one. I.e. it affects the reader, whereas this patch just takes a first cut at reducing what we emit in the writer. So I am personally happy with this one going in first. Peter, are there things here that would be throwaway work if this went in first and that was a follow on?


https://reviews.llvm.org/D35334