[Bitcode][Asm] Parse metadata value from operand bundles

[Prabhuk]

Summary

Support parsing operand bundles with metadata value from LLVM IR.

This is almost an NFC since there is no existing producers/consumers for
metadata operand bundle values, except the following call graph section
patches in revision:

Producer: https://reviews.llvm.org/D105909
Consumer: https://reviews.llvm.org/D105915

The test in this revision only ensures that the parser does not
complain. The above-mentioned patches implicity test if passed metadata
value is correct.

[necipfazil]

The reason why metadata values cannot be explicity tested is that we cannot read them out. See D107039. A potential fix to D107039 might involve BitcodeReader/Writer.

This revision fixes the parser, and the producer/consumers to metadata operand values (D105909 and D105915) implicity tests that the values are read correctly.

[morehouse]

llvm/lib/AsmParser/LLParser.cpp

2749

Previously we returned true in this case. Why can we return false now?

[necipfazil]

Return error if operand bundle value type cannot be parsed

Also, add a FIXME for the issue with reading out metadata operand bundle from
bitcode as discussed in D107039.

[morehouse]

llvm/lib/AsmParser/LLParser.cpp

2753

Please also link the bug here.

[necipfazil]

Add reference to the bug report for reading metadata operand bundle values out

[MaskRay]

llvm/test/Bitcode/2021-07-22-Parse-Metadata-Operand-Bundles.ll

1

The practice of using 2021-07-22- for tests was probably dropped 10 years ago.
New tests shouldn't follow such patterns.

2

This just checks that it assembles but no actual verification is performed.

[MaskRay]

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)

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

[dexonsmith]

In 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.

llvm/lib/AsmParser/LLParser.cpp

2753

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.

llvm/test/Bitcode/2021-07-22-Parse-Metadata-Operand-Bundles.ll

2

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).

; RUN: llvm-as <%s | llvm-dis | llvm-as | llvm-dis | FileCheck %s

[Prabhuk]

https://discourse.llvm.org/t/rfc-computing-storing-and-restoring-conservative-call-graphs-with-llvm/58446/3?u=prabhuk

[Prabhuk]

Rebased the patchset and addressed the compilation failures

llvm/lib/AsmParser/LLParser.cpp

while (Lex.getKind() != lltok::rsquare) {
     std::vector<Value *> Inputs;
     while (Lex.getKind() != lltok::rparen) {
       // If this isn't the first input, we need a comma.
       if (!Inputs.empty() &&
           parseToken(lltok::comma, "expected ',' in input list"))
         return true;
 
       Type *Ty = nullptr;
+      if (parseType(Ty))
+        return true;
+
       Value *Input = nullptr;
-      if (parseType(Ty) || parseValue(Ty, Input, PFS))
+      // FIXME: Metadata operand bundle value is garbage when LLVM IR is
+      // compiled to bitcode, then disassembled back to LLVM IR. See D107039
+      // for the reproducers, and https://bugs.llvm.org/show_bug.cgi?id=51264
+      // for the bug report.
+      if (Ty->isMetadataTy()) {
+        if (parseMetadataAsValue(Input, PFS))
           return true;
+      } else {
+        if (parseValue(Ty, Input, PFS))
+          return true;
+      }
       Inputs.push_back(Input);
     }
 
     BundleList.emplace_back(std::move(Tag), std::move(Inputs));
 
     Lex.Lex(); // Lex the ')'.
   }
 

llvm/test/Bitcode/2021-07-22-Parse-Metadata-Operand-Bundles.ll

+; This test ensures that we parse metadata operand bundle values.
+; RUN: llvm-as < %s
+
+declare void @callee()
+
+define void @call_with_operand_bundle() {
+  call void @callee() [ "op_type"(metadata !"metadata_string") ]
+  ret void
+}