[PATCH] D35334: ThinLTO Minimized Bitcode File Size Reduction

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:124
   /// be used in the backend.
   ModuleHash *ModHash;
 
----------------
haojiewang wrote:
> haojiewang wrote:
> > haojiewang wrote:
> > > pcc wrote:
> > > > haojiewang wrote:
> > > > > pcc wrote:
> > > > > > I think this change removes the need to support writing a module with a precomputed hash from ModuleBitcodeWriter. Can you please remove that functionality?
> > > > > Writing a module with a precomputed hash is still needed while split writing a module(splitAndWriteThinLTOBitcode), because I didn't touch the split writing in this patch. Maybe I should also change the writer in split writing? 
> > > > You mean here?
> > > > http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp#386
> > > > 
> > > > Yes, I think you can change that to call writeThinLinkBitcode.
> > > I just tried to change the split writing, but there were bunch of errors in test after I did that. There will be 11 test files should be updated if we change this, and most of them are because llvm-dis cannot read a thin link bitcode file. Should we do it in this patch or leave it the next patch? How should we update them, just changing llvm-dis to llvm-bcanalyzer or also adding the CHECKs and CHECK-NOTs as in the test file of this patch?
> > I'm just not sure if I would affect other people's code if I do so much changes on the test files.
> Sorry, I just made a terrible mistake on this one. Actually there would be only 1 test file should be updated, which is for the llvm-dis problem. Besides, for the "NODEBUG" checks, I can just remove them because they are not needed after this patch, right? And after I add the CHECKs and CHECK-NOTs in distributed-import.ll, I don't have to do the check again for the split.ll test file, right?
Yes, you can remove the NODEBUG checks from that file, and no other changes should be required there since we're already making sure we can read the resulting files with llvm-lto (ideally it should be changed to use llvm-lto2, but that can be done some other time).


https://reviews.llvm.org/D35334