Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Prabhuk
Copy link
Contributor

@Prabhuk Prabhuk commented Apr 3, 2024

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?id=358327
Consumer: https://reviews.llvm.org/D105915?id=358335

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.

Created using spr 1.3.6-beta.1
Prabhuk pushed a commit to Prabhuk/llvm-project that referenced this pull request Apr 19, 2024
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?id=358327
Consumer: https://reviews.llvm.org/D105915?id=358335

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.

Reviewed By:
morehouse

Differential Revision: https://reviews.llvm.org/D107038?id=362649

Pull Request: llvm#87570
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@Prabhuk Prabhuk requested review from petrhosek and ilovepi April 24, 2024 16:56
Created using spr 1.3.6-beta.1
Comment on lines 3143 to 3144
// compiled to bitcode, then disassembled back to LLVM IR. See PR#89649
// for the reproducers, and https://bugs.llvm.org/show_bug.cgi?id=51264
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// compiled to bitcode, then disassembled back to LLVM IR. See PR#89649
// for the reproducers, and https://bugs.llvm.org/show_bug.cgi?id=51264
// compiled to bitcode, then disassembled back to LLVM IR.
// See [PR#89649](https://github.com/llvm/llvm-project/pull/89649)
// for the reproducers, and https://github.com/llvm/llvm-project/issues/50608

I think URLs are better here, since developers can use them directly. The old bugzilla bug also has been migrated to github, so I think its better to use that link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to only add support for one half of this... If we add IR parsing support, then bitcode writing should also work.

@@ -0,0 +1,9 @@
; This test ensures that we parse metadata operand bundle values.
; RUN: llvm-as < %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No CHECK lines? is this just a test that llvm-as doesn't crash?

necipfazil added 3 commits May 1, 2024 15:40
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was already implemented in #110805.

@ilovepi
Copy link
Contributor

ilovepi commented Nov 14, 2024

Yeah, I don't think this was rebased in several months, otherwise, I'd expect a conflict between ToT and this patch.

If the comment about round tripping the operand bundle through bitcode and back to IR is still relevant, we should probably add a test, and update any bug tracking though. @Prabhuk would you mind following up on that part, since it seems like the functionality you're after is already in tree?

@Prabhuk
Copy link
Contributor Author

Prabhuk commented Nov 20, 2024

Yeah, I don't think this was rebased in several months, otherwise, I'd expect a conflict between ToT and this patch.

If the comment about round tripping the operand bundle through bitcode and back to IR is still relevant, we should probably add a test, and update any bug tracking though. @Prabhuk would you mind following up on that part, since it seems like the functionality you're after is already in tree?

I've dropped this PR from my patchset. I'll verify if my reproducer (#89649) to test if the round tripping the operand bundle to bitcode and back to IR works correctly on top of main. If required, I'll update the tracking bug, if not I'll close this PR out.

@Prabhuk Prabhuk closed this May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants