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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
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;
----------------
tejohnson wrote:
> 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.
> Alternatively, emit into its own block, then you can emit the word offset.

Thinking more about this, it should be lower overhead to emit in the original block and just emit properly as a 64-bit value. I think that is just invoking Emit64 without the cast on V, and adding a new backpatch interface that takes a 64-bit unsigned and splits into two halves as Emit64 does in the >32 bit case.


https://reviews.llvm.org/D28083