[PATCH] D35334: ThinLTO Minimized Bitcode File Size Reduction

Peter Collingbourne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 12 18:33:22 PDT 2017


pcc added inline comments.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:4031
+
+  writeModuleInfo();
+
----------------
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.


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/no-type-md.ll:40
 ; CHECK: !llvm.dbg.cu
 ; NODEBUG-NOT: !llvm.dbg.cu
+; NODEBUG-NOT: IDENTIFICATION_BLOCK_ID
----------------
haojiewang wrote:
> pcc wrote:
> > This part of the test is no longer relevant.
> You mean that I'd better write a new test file, right? Along with the llvm-lto2 test.
I just mean that if you're now reading the file with llvm-bcanalyzer instead of llvm-dis you wouldn't expect the output to contain `!llvm.dbg.cu` either with or without your change, so it doesn't make sense to test for it. You can keep the rest of the test in this file.

Regarding llvm-lto2, you could either create a new test file or convert this one to use llvm-lto2 instead of llvm-lto.


https://reviews.llvm.org/D35334





More information about the llvm-commits mailing list