[PATCH] D35334: ThinLTO Minimized Bitcode File Size Reduction

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: include/llvm/Bitcode/BitcodeWriter.h:93
+    void writeThinLinkBitcode(const Module *M, const ModuleSummaryIndex &Index, 
+                      ModuleHash &ModHash);
+
----------------
clang-format


================
Comment at: include/llvm/Bitcode/BitcodeWriter.h:134
+  void WriteThinLinkBitcodeToFile(const Module *M, raw_ostream &Out,
+                          const ModuleSummaryIndex &Index,
+                          ModuleHash &ModHash);
----------------
clang-format


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:114
   /// True if a module hash record should be written.
   bool GenerateHash;
 
----------------
I think we can move this into the derived class now, together with Hasher and addToStrtab.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:124
   /// be used in the backend.
   ModuleHash *ModHash;
 
----------------
I think this change removes the need to support writing a module with a precomputed hash from ModuleBitcodeWriter. Can you please remove that functionality?


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:142
+  ModuleBitcodeWriterBase(const Module *M, StringTableBuilder &StrtabBuilder,
                       BitstreamWriter &Stream, bool ShouldPreserveUseListOrder,
                       const ModuleSummaryIndex *Index, bool GenerateHash,
----------------
clang-format


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/no-type-md.ll:40
 ; CHECK: !llvm.dbg.cu
 ; NODEBUG-NOT: !llvm.dbg.cu
+; NODEBUG-NOT: IDENTIFICATION_BLOCK_ID
----------------
pcc wrote:
> haojiewang wrote:
> > pcc wrote:
> > > haojiewang wrote:
> > > > pcc wrote:
> > > > > haojiewang wrote:
> > > > > > pcc wrote:
> > > > > > > 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.
> > > > > > Current thin link bitcode cannot be read by llvm-lto2. I'm tracing the reason and trying to find how to fix it. Do we need fix this in this patch?
> > > > > How are you calling llvm-lto2? You're passing `-thinlto-distributed-indexes`, right?
> > > > Actually no, I just used the same command I saw in other test because I'm now familiar with llvm-lto2. What test command should I use?
> > > Probably something like http://llvm-cs.pcc.me.uk/test/ThinLTO/X86/distributed_import.ll#10 with resolutions for f and g.
> > llvm-lto2 works, but this test cannot past. Because the symtab will not be built in this test case. And the full IR bitcode file for this test also cannot pass llvm-lto2 because there is no data layout in this test. Should I change this test to a more complete one or just leave it here and add another test?
> I'd add a triple and datalayout to this test, it probably ought to have one anyway.
Sorry, I clearly wasn't paying attention when I referred you to the distributed_import.ll test above, as that test is testing for pretty much exactly what I thought wasn't already being tested for. May I ask you to remove the llvm-lto2 part and move the `llvm-bcanalyzer` part of this test to distributed_import.ll?


https://reviews.llvm.org/D35334