search

cdisselkoen / llvm-ir

LLVM IR in natural Rust data structures
MIT License
539 stars 45 forks source link

LLVM 16 support #39

Closed woodruffw closed 11 months ago

woodruffw commented 1 year ago

WIP; closes #38.

woodruffw commented 1 year ago

The main TODO here is support for target extension types: https://releases.llvm.org/16.0.0/docs/LangRef.html#target-extension-type

Benjins commented 11 months ago

Hi there: thanks for all the work on the upgrade to LLVM 16! This PR has been a draft for a while, so I made a PR to your fork which should get the tests all passing (along with a merge from latest main). If you're okay with that, then the PR should (I think) be ready for review. Or if you want, I can pick up the upgrade work and make a new PR, if you have a lot else on your plate

Also, as you point out, Target Extension Types are not currently exposed to the C API, so until a future upgrade they won't be implementable. I don't know exactly how we should deal with them in the meantime, but my own opinion is that they shouldn't block this initial upgrade

Link to my PR for this fork: https://github.com/trail-of-forks/llvm-ir/pull/1

woodruffw commented 11 months ago

Hi @Benjins, thanks! I'll take a look at your PR now; I greatly appreciate you picking it up.

woodruffw commented 11 months ago

Merged; I'll also get this updated against the upstream main. Thanks again @Benjins!

woodruffw commented 11 months ago

Tests for this are mostly passing for me now, although I'm getting a SIGTRAP on llvm_10_tests (this doesn't make any sense to me, but I haven't looked closely yet).

Benjins commented 11 months ago

I'm getting a SIGTRAP on llvm_10_tests

If you're running tests with a debug rust build (i.e. not with the --release flag), there is an existing issue where LLVM's C API does not properly handle memory orders for fence instructions. I tend to test in release mode to avoid the crash. See https://github.com/cdisselkoen/llvm-ir/issues/44

woodruffw commented 11 months ago

Funny bug! Yeah, confirmed: --release fixes it, and all tests pass locally for me now.

Benjins commented 11 months ago

cc @cdisselkoen Not sure if you get a notification when a draft PR is un-drafted, but this PR should be ready for review. There are some follow-up changes that we'll probably want, and there is the question of what to do with Target Extension Types (not currently available via the C API). But this should provide initial LLVM 16 support that can be built on

cdisselkoen commented 11 months ago

Looks great. Thanks for everyone's work.

For follow-up, I think we can handle Target Extension Types much like we do things like BlockAddress today -- basically just have a marker that contains no data since we can't get any data from the C API.