[PATCH] D28083: Add an index for Module Metadata record in the bitcode

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I assume https://reviews.llvm.org/D28113 is dependent on this? Code changes look fine (couple questions and comment nits below). Looks like this will be worth it from a time improvement standpoint, but what is the size overhead?



================
Comment at: llvm/include/llvm/Bitcode/BitstreamWriter.h:282
       if (Op.getEncodingData())
-        Emit((unsigned)V, (unsigned)Op.getEncodingData());
+        Emit64((unsigned)V, (unsigned)Op.getEncodingData());
       break;
----------------
This change seems unnecessary


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1887
 
-  Stream.EnterSubblock(bitc::METADATA_BLOCK_ID, 3);
+  Stream.EnterSubblock(bitc::METADATA_BLOCK_ID, 4);
   SmallVector<uint64_t, 64> Record;
----------------
What change necessitated increasing this from 3 to 4 (I forget exactly how this is determined - is it the number of codes or the number of abbrevs?).


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1934
+  // efficiently.
+  Stream.BackpatchWord(BeginBlockBitPos - 64, // FIXME: comment the 54
+                       Stream.GetCurrentBitNo() - BeginBlockBitPos);
----------------
Fix the FIXME? Also I assume it should be "comment the 64"?


================
Comment at: llvm/test/Bitcode/mdnodes-in-post-order.ll:33
 
+; Before the named records we emit an offset to the index for the previous records
+; CHECK-NEXT:   <INDEX
----------------
Isn't this emitting an offset to the metadata records, not to the index? Ditto for comments for INDEX block in other tests.


https://reviews.llvm.org/D28083