[PATCH] D35334: ThinLTO Minimized Bitcode File Size Reduction

[Teresa Johnson via llvm-commits]

On Thu, Jul 13, 2017, 11:02 AM Peter Collingbourne via Phabricator <
reviews at reviews.llvm.org> wrote:

> pcc added inline comments.
>
>
> ================
> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:4031
> +
> +  writeModuleInfo();
> +
> ----------------
> tejohnson wrote:
> > 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?
> I can accept that it may be easier to develop the patches in this order,
> but as I mentioned it makes things harder to review (which I guess is the
> "throwaway work" -- if we go with this first I would need to carefully look
> at the body of writeModuleInfo to make sure that running it wouldn't cause
> problems in this new flow, even though we know that it won't make a
> difference in the end).
>

Ok, if that routine needs careful vetting regardless, then I agree it makes
sense to do it all in one patch.


> Generally when I write patches I try to make things easier for the
> reviewer. But as I say, up to you folks.
>
>
> https://reviews.llvm.org/D35334
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170713/de4f0917/attachment.html>