[PATCH] D107038: [Bitcode][Asm] Parse metadata value from operand bundles

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D107038#2915295 <https://reviews.llvm.org/D107038#2915295>, @MaskRay wrote:

> Added @dexonsmith who reviewed the original Operand Bundles patch and can suggest how to properly this (you are right that the dependent patches implicitly test this but this really needs something more than `; RUN: llvm-as < %s`)

Right, I think if we're going to support metadata in operand bundles it needs to work in textual IR //and// bitcode. (Probably an oversight that the API doesn't assert on this case...)

> This needs a documentation update on https://llvm.org/docs/LangRef.html#operand-bundles

Also an RFC to llvm-dev please, since this is an IR change! I can see how this could be useful but it's valuable to have extra eyes on changes like this.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:2753
+      // FIXME: Metadata operand bundle value is garbage when LLVM IR is
+      // compiled to bitcode, then disassembled back to LLVM IR. See D107039.
+      if (Ty->isMetadataTy()) {
----------------
morehouse wrote:
> Please also link the bug here.
Hmm... we don't usually link to phab reviews or bugs in code, relying on `git blame` for finding those.  But in either case I think this patch ought to get bitcode working at the same time it gets textual IR.


================
Comment at: llvm/test/Bitcode/2021-07-22-Parse-Metadata-Operand-Bundles.ll:2
+; This test ensures that we parse metadata operand bundle values.
+; RUN: llvm-as < %s
+
----------------
MaskRay wrote:
> This just checks that it assembles but no actual verification is performed.
I'd suggest a double round-trip (one round-trip is enough for the `CHECK` lines; the second ensures that what `llvm-dis` emits can also be parsed).
```
lang=llvm
; RUN: llvm-as <%s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107038/new/

https://reviews.llvm.org/D107038