Preliminary LLVM 19/20 support#557
Conversation
|
I used a simple JIT example from README and it works. Looking good already but still need unit tests |
|
@TheDan64 Should the feature base name be renamed as llvm19-1? Looks like LLVM skipped 19.0 entirely and released 19.1 as the first one |
|
Nah, it's fine and consistent as is 🤔 |
|
That is such a weird choice... |
|
Yeah, I guess 19-1 makes more sense |
|
@TheDan64 okay, I also tested LLVM 19 integration with https://github.com/jamesmth/llvm-plugin-rs, so the basic examples provided in their repo do compile pretty well, so I will rename the feature from Keep in mind the new features in LLVM 19 aren't added yet, and so a follow up contribution for completing that puzzle would be a huge welcome. |
I believe LLVM skipped 18.0 as well, and inkwell uses 18-0 for the major feature number, so it would be more consistent to use 19-0 now as well. |
No the problem is LLVM directly skipped 19.0 and even llvm-sys have to use 191 as the prefix number, so it is the matter of consistency in the library or in the actual LLVM naming scheme |
|
I have been able to integrate stevefan1999-personal@0c1e5dd into my project that relies on Inkwell, but stevefan1999-personal@87b7049 creates a number of compilation errors about undefined feature version For the version with the successful integration everything seems to be working swimmingly so far, with no regressions found by the test suite. |
That is strange, let me revert the actions changes tonight |
|
I was also able to get Simply changing the macro occurrences of "19" to "19.1" seems to solve this issue. Here's the patch (unified diff): |
|
Hi @stevefan1999-personal, are you able to address the above issues? |
Ah I was busy with company stuff, let me handle that tonight |
I guess the biggest problem is that we don't know why LLVM skipped the whole 19.0 release and go straight to 19.1, which the 19.. range should technically be covered. I think as a workaround, let me merge it to be using 19.1 then, but keep in mind this is pretty abnormal in a common sense |
|
https://github.com/stevefan1999-personal/inkwell/actions/runs/13450580072/job/37584222806 Looks like 19.1 amd64 is missing from KyleMayes and one of the PR that attempts to fix it is still missing it as well...Can't really test 19.1 at the moment with CI for now |
|
@stevefan1999-personal do you suggest waiting to merge until 19.1 is added? |
|
@TheDan64 all tests passed on my actions, so if it is LGTY let's merge it ASAP so I can work on the llvm-plugin-rs |
|
I wont be able to review this until next week |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces preliminary support for LLVM 19 and 20 by adding new feature flags across various type and module definitions, updating CI workflows, and configuring Cargo dependencies to accommodate the newer LLVM versions.
- Extended feature lists in type definitions and trait implementations
- Updated CI configuration and documentation to include LLVM 19/20 installations
- Adjusted examples and context functions to reflect these new LLVM versions
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/*.rs | Added LLVM 19/20 feature flags in type modules |
| src/lib.rs | Updated extern crate declarations for LLVM 19/20 support |
| src/debug_info.rs | Introduced new blocks with todo!() for LLVM 19/20 debug info stubs |
| src/context.rs, src/builder.rs | Updated function implementations and overloads for new LLVM versions |
| internal_macros/src/cfg.rs | Updated feature list to include llvm19-1 and llvm20-1 |
| examples/kaleidoscope/main.rs, Cargo.toml, .github/workflows/test.yml | Adapted examples, documentation, and CI for LLVM 19/20 integration |
src/debug_info.rs
Outdated
| #[cfg(any(feature = "llvm19-1", feature = "llvm20-1"))] | ||
| { | ||
| let _ = value_ref; | ||
| todo!() |
There was a problem hiding this comment.
The use of todo!() in the DebugInfoBuilder functions for LLVM 19/20 may lead to unexpected panics. Consider adding a clear error message or a fallback implementation along with documentation to indicate that these functions are currently unimplemented.
There was a problem hiding this comment.
Tell me how to use the new API for Debug Struct starting from LLVM 19 as a replacement to old API, please?
There was a problem hiding this comment.
Okay I did not see that Copilot won't reply to this thread, so I thought I have a prompt for that there...
There was a problem hiding this comment.
I don't follow, can't you leave the current api as is?
If there is a new api we should either support it here or in a future PR without removing existing functionality (assuming the methods haven't been removed)
| #[llvm_versions(..17)] | ||
| pub fn build_int_nuw_neg<T: IntMathValue<'ctx>>(&self, value: T, name: &str) -> Result<T, BuilderError> { | ||
| if self.positioned.get() != PositionState::Set { | ||
| return Err(BuilderError::UnsetPosition); | ||
| } | ||
| let c_string = to_c_str(name); | ||
| let value = unsafe { LLVMBuildNUWNeg(self.builder, value.as_value_ref(), c_string.as_ptr()) }; | ||
| unsafe { Ok(T::new(value)) } | ||
| } | ||
|
|
||
| // SubType: <I>(&self, value: &IntValue<I>, name) -> IntValue<I> { | ||
| #[llvm_versions(17..)] | ||
| pub fn build_int_nuw_neg<T: IntMathValue<'ctx>>(&self, value: T, name: &str) -> Result<T, BuilderError> { | ||
| if self.positioned.get() != PositionState::Set { | ||
| return Err(BuilderError::UnsetPosition); | ||
| } | ||
| let c_string = to_c_str(name); | ||
| let value = unsafe { LLVMBuildNeg(self.builder, value.as_value_ref(), c_string.as_ptr()) }; | ||
| unsafe { | ||
| LLVMSetNUW(value, true.into()); | ||
| } |
There was a problem hiding this comment.
[nitpick] Two versions of build_int_nuw_neg are provided for different LLVM versions. Consider refactoring or clearly documenting the differences to improve code clarity and reduce potential confusion in maintenance.
| #[llvm_versions(..17)] | |
| pub fn build_int_nuw_neg<T: IntMathValue<'ctx>>(&self, value: T, name: &str) -> Result<T, BuilderError> { | |
| if self.positioned.get() != PositionState::Set { | |
| return Err(BuilderError::UnsetPosition); | |
| } | |
| let c_string = to_c_str(name); | |
| let value = unsafe { LLVMBuildNUWNeg(self.builder, value.as_value_ref(), c_string.as_ptr()) }; | |
| unsafe { Ok(T::new(value)) } | |
| } | |
| // SubType: <I>(&self, value: &IntValue<I>, name) -> IntValue<I> { | |
| #[llvm_versions(17..)] | |
| pub fn build_int_nuw_neg<T: IntMathValue<'ctx>>(&self, value: T, name: &str) -> Result<T, BuilderError> { | |
| if self.positioned.get() != PositionState::Set { | |
| return Err(BuilderError::UnsetPosition); | |
| } | |
| let c_string = to_c_str(name); | |
| let value = unsafe { LLVMBuildNeg(self.builder, value.as_value_ref(), c_string.as_ptr()) }; | |
| unsafe { | |
| LLVMSetNUW(value, true.into()); | |
| } | |
| pub fn build_int_nuw_neg<T: IntMathValue<'ctx>>(&self, value: T, name: &str) -> Result<T, BuilderError> { | |
| if self.positioned.get() != PositionState::Set { | |
| return Err(BuilderError::UnsetPosition); | |
| } | |
| let c_string = to_c_str(name); | |
| let value = unsafe { | |
| #[cfg(llvm_versions = "..17")] | |
| { | |
| LLVMBuildNUWNeg(self.builder, value.as_value_ref(), c_string.as_ptr()) | |
| } | |
| #[cfg(llvm_versions = "17..")] | |
| { | |
| let v = LLVMBuildNeg(self.builder, value.as_value_ref(), c_string.as_ptr()); | |
| LLVMSetNUW(v, true.into()); | |
| v | |
| } | |
| }; |
TheDan64
left a comment
TheDan64
left a comment
There was a problem hiding this comment.
This looks pretty good but there are a couple things that need to be addressed before merging
|
|
||
| * Rust 1.56+ (Stable, Beta, or Nightly) | ||
| * One of LLVM 8-18 | ||
| * One of LLVM 8-18 (WIP: 19 and 20 are barely supported without extensive tests) |
There was a problem hiding this comment.
I specifically mentioned 19 and 20 because both of them are indeed not tested especially with their new APIs, thus I should call this >=19 maybe?
There was a problem hiding this comment.
If the test suite passes for those versions we consider them supported, even if we don't have new tests specifically for them
src/debug_info.rs
Outdated
| #[cfg(any(feature = "llvm19-1", feature = "llvm20-1"))] | ||
| { | ||
| let _ = value_ref; | ||
| todo!() |
There was a problem hiding this comment.
This looks like it still needs to be fixed? And a few later on as well
There was a problem hiding this comment.
I have no idea on how to use the new Debug Record API starting from LLVM 19, I think it is not just deprecated is it removed entirely? Or we can just version it out but it will be very confusing to end users
There was a problem hiding this comment.
Yeah I guess that new API deserve a whole new struct and tailor-made wrapper APIs around it to be honest.
There was a problem hiding this comment.
If the methods don't exist then we must version them out, otherwise keep them. Adding new APIs can be done in a separate PR
… the official apt packages for 19 and above KyleMayes/install-llvm-action uses the semi-official binaries uploaded to the LLVM github releases. For LLVM 19, these are built with LTO, which cannot be linked against by the standard linkers (and broke CI for LLVM 19). To work around this, we can use the github action to install older versions of LLVM, but use the official apt packages for newer versions. Ubuntu 24.04 (noble) has packages starting at LLVM 17, so this should be good for a while. This is pretty much what Inkwell seems to be doing as well: TheDan64/inkwell#557
… the official apt packages for 19 and above KyleMayes/install-llvm-action uses the semi-official binaries uploaded to the LLVM github releases. For LLVM 19, these are built with LTO, which cannot be linked against by the standard linkers (and broke CI for LLVM 19). To work around this, we can use the github action to install older versions of LLVM, but use the official apt packages for newer versions. Ubuntu 24.04 (noble) has packages starting at LLVM 17, so this should be good for a while. This is pretty much what Inkwell seems to be doing as well: TheDan64/inkwell#557
… the official apt packages for 19 and above KyleMayes/install-llvm-action uses the semi-official binaries uploaded to the LLVM github releases. For LLVM 19, these are built with LTO, which cannot be linked against by the standard linkers (and broke CI for LLVM 19). To work around this, we can use the github action to install older versions of LLVM, but use the official apt packages for newer versions. Ubuntu 24.04 (noble) has packages starting at LLVM 17, so this should be good for a while. This is pretty much what Inkwell seems to be doing as well: TheDan64/inkwell#557
… the official apt packages for 19 and above KyleMayes/install-llvm-action uses the semi-official binaries uploaded to the LLVM github releases. For LLVM 19, these are built with LTO, which cannot be linked against by the standard linkers (and broke CI for LLVM 19). To work around this, we can use the github action to install older versions of LLVM, but use the official apt packages for newer versions. Ubuntu 24.04 (noble) has packages starting at LLVM 17, so this should be good for a while. This is pretty much what Inkwell seems to be doing as well: TheDan64/inkwell#557
|
@stevefan1999-personal are you able to resolve the remaining issues and comments so we can merge this PR? |
|
@TheDan64 let me take a look this weekend |
|
@TheDan64 do you think it is good to go |
|
@stevefan1999-personal there are some clippy failures... if you do |
|
@TheDan64 would you be willing to make a new release with this PR included? |
|
Sure thing |
|
0.7.0 milestone has been triaged now accurately represents what remains before release: https://github.com/TheDan64/inkwell/milestone/9 |
… the official apt packages for 19 and above (#76) KyleMayes/install-llvm-action uses the semi-official binaries uploaded to the LLVM github releases. For LLVM 19, these are built with LTO, which cannot be linked against by the standard linkers (and broke CI for LLVM 19). To work around this, we can use the github action to install older versions of LLVM, but use the official apt packages for newer versions. Ubuntu 24.04 (noble) has packages starting at LLVM 17, so this should be good for a while. This is pretty much what Inkwell seems to be doing as well: TheDan64/inkwell#557
Description
Adds preliminary LLVM 19/20 support so that https://github.com/jamesmth/llvm-plugin-rs can build plugin with it. No new features have been added so far.
I've also added
nextestand replaced some CI with caching, so now the CI pipeline for testing is faster now.No new LLVM 19 features have been tested, hence "preliminary" in the sense that it is not a very complete port, but it should be at least backward compatible with existing LLVM API
Related Issue
#530
How This Has Been Tested
Not yet
Option<Breaking Changes>
Checklist