This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Prabhuk on Jul 28 2021, 11:07 PM.

Details

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.

Diff Detail

Event Timeline

necipfazil created this revision.Jul 28 2021, 11:07 PM
necipfazil requested review of this revision.Jul 28 2021, 11:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2021, 11:07 PM

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 added inline comments.Jul 29 2021, 1:39 PM
llvm/lib/AsmParser/LLParser.cpp
2749 ↗(On Diff #362649)

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

necipfazil marked an inline comment as done.

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 accepted this revision.Jul 29 2021, 3:50 PM
morehouse added inline comments.
llvm/lib/AsmParser/LLParser.cpp
2753 ↗(On Diff #362902)

Please also link the bug here.

This revision is now accepted and ready to land.Jul 29 2021, 3:50 PM
necipfazil marked an inline comment as done.

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

morehouse accepted this revision.Jul 29 2021, 4:18 PM
MaskRay added inline comments.
llvm/test/Bitcode/2021-07-22-Parse-Metadata-Operand-Bundles.ll
1 ↗(On Diff #362918)

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

2 ↗(On Diff #362918)

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

MaskRay requested changes to this revision.Jul 29 2021, 5:02 PM
This revision now requires changes to proceed.Jul 29 2021, 5:02 PM

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

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 ↗(On Diff #362902)

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 ↗(On Diff #362918)

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
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 22, 1:53 PM
Prabhuk updated this revision to Diff 558150.Wed, Nov 22, 2:09 PM

Rebased the patchset and addressed the compilation failures

phosek added a subscriber: phosek.Mon, Nov 27, 9:53 AM