[PATCH] D12457: [Bitcode][Asm] Teach LLVM to read and write operand bundles.

[Sanjoy Das via llvm-commits]

 > I think you can disable the `verify-uselistorder` run lines for now.
 > Something like:
 > --
 > RUN-PR24755: ...
 > --
 > makes sense to me (and then please document this in the PR so that we
 > remember to enable the tests when the PR gets fixed).

Done.  Now this change passes ninja check.

I'll add the revision number to the PR when I check this in.

 > This uses a lot of new bitcode records.  I think you can do it with just
 > one new function record, and simplify some of the logic in the process.
 >
 > Firstly, you can avoid the new forms of CALL/INVOKE by emitting the
 > BUNDLE records *before* the instructions instead of after.  The reader
 > logic would be something like this:
 > --
 > SmallVector<Bundle, 8>  Bundles;
 > while (1) {
 >    Bitcode = ...;
 >
 >    // Emit an error if bundles attached to invalid instruction.
 >    if (!Bundles.empty()&&
 >        Bitcode != BUNDLE&&
 >        Bitcode != CALL&&
 >        Bitcode != INVOKE)
 >      return Error("Bundles attached to invalid instruction");
 >
 >    // Old switch statement to read records.
 >    switch (Bitcode) {
 >    case BUNDLE:
 >      Bundles.emplace_back(...);
 >      break;
 >    case CALL:
 >      // ...
 >      ... = CreateCall(..., Bundles);
 >      break;
 >    case ...
 >    }
 > }
 > --
 > In the writer, you just emit the bundles before emitting the rest of the
 > instruction to accomplish this.
 >
 > Secondly, now that the bundles precede the instruction they're being
 > attached to, you don't need the BUNDLES_HEADER record.  The number of
 > bundles is recorded implicitly.
 >
 > Thirdly, you can merge the BUNDLE_TAG and BUNDLE_CONTENTS records by
 > adding a field for the number of values.
 > --
 > // BUNDLE: [ V, V x vals, L x char ]
 > --
 >
 > Even better: I assume a module is only likely to have a small-ish set of
 > bundle tags that are reused?  If so, you can add a BUNDLE_BLOCK (or
 > some such block) that defines the tags, and emit it at the top of the
 > module:
 > --
 > // BUNDLE_TAG: [ n x strchr ]
 > --
 > This implicitly numbers the tags by the order of the records.
 >
 > One benefit is that each tag is only serialized once, but this also
 > simplifies (and shrinks) the BUNDLE records, which can refer to the tags
 > by ID.
 > --
 > // BUNDLE: [ tag, n x vals ]
 > --

Thanks for the detailed review!

I've changed the bitcode reading / writing to use this prefix scheme.
This second revision of the change introduces one new kind of block
(OPERAND_BUNDLE_TAGS_BLOCK_ID) and one new kind of function code
(FUNC_CODE_OPERAND_BUNDLE).

 >>     enum UseListCodes {
 >> Index: docs/LangRef.rst
 >> ===================================================================
 >> --- docs/LangRef.rst
 >> +++ docs/LangRef.rst
 >> @@ -1438,7 +1438,32 @@
 >>       the ELF x86-64 abi, but it can be disabled for some compilation
 >>       units.
 >>
 >> -.. _moduleasm:
 >
 > Did you mean to delete this anchor?

No, fixed.

 >> +Operand bundles are a generic mechanism intended to support
 >> +runtime-introspection like functionality for managed languages.  The
 >
 > I would parse this better if "like" were hyphenated, as in "hyphen-like
 > syntax".

Fixed.

 >
 >> +exact semantics of an operand bundle is dependent on the bundle tag.
 >
 > (I'm assuming you'll describe the tags here as they're added?)

Yes.  I also intend to add some more documentation before checking in
(so I've added a TODO here).

-- Sanjoy