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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I guess you don't have any BitcodeReader changes since the unknown records are simply ignored? I think it would be useful to add a BitcodeReader change though to parse the index offset record and sanity check against the offset of the index record when it is later encountered.



================
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;
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > This change seems unnecessary
> The offset that we back patch is emitted as a 64 bits fixed size, which wasn't possible without this change.
But you are casting V to unsigned aka unsigned int, which is 32 bits. It seems like you don't want to cast V at all if you change to Emit64, since Emit64 takes a uint64_t, and in order to pass in a 64-bit val the uintty of V should already be uint64_t. This is similar to how V is not cast when passed to EmitVBR64 below (which takes a uint64_t value).

Actually - I think you are getting lucky here because BackpatchWord also takes a 32-bit unsigned value. So you are never currently writing a 64-bit value for the offset.

For the VST offset we simply emit the number of 32-bit words since blocks are guaranteed to be 32-bit aligned. I guess you can't do that here since the index is not at the start of the block. Alternatively, emit into its own block, then you can emit the word offset.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1921
+  // patched when we emit the index later. We can simply subtract the 64-bit
+  // fixed size from the current bit number to get the location to backpatch.
+  uint64_t BeginBlockBitPos = Stream.GetCurrentBitNo();
----------------
mehdi_amini wrote:
> The 64 is documented here, do you thinks it is worth repeating the comment below?
I don't think you need to document it again below.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1938
+  // fixed size from the current bit number to get the location to backpatch.
+  uint64_t BeginBlockBitPos = Stream.GetCurrentBitNo();
+
----------------
Not the beginning of the block, so variable name should be changed.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1951
+  Stream.BackpatchWord(BeginBlockBitPos - 64,
+                       Stream.GetCurrentBitNo() - BeginBlockBitPos);
+
----------------
As noted earlier, this value gets passed to a 32-bit unsigned parameter. You'll need a new interface to write a 64-bit offset, or write 2 32-bit halves.


https://reviews.llvm.org/D28083