-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Update to LLVM 19 #127513
Update to LLVM 19 #127513
Conversation
This comment has been minimized.
This comment has been minimized.
@bors try |
Update to LLVM 19 r? `@ghost`
☀️ Try build successful - checks-actions |
@rust-timer build d009d2d |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (d009d2d): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -3.5%, secondary 1.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -5.4%, secondary -3.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.6%, secondary 1.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 702.485s -> 689.195s (-1.89%) |
I believe this is caused by llvm/llvm-project#88492, which now seems to ignore the I'm a bit unclear on what the implications of that change on Rust are. Are we supposed to switch all our wasm targets to |
That sounds like a mistake, because the plan is for Another point to add is that I guess a third solution would be to |
To clarify my previous comment, because I think it didn't get across right: If I understand correctly, if |
Yes, but So this would mean if you added |
Ah, I see what you mean now. So I guess if we wanted to keep the status quo, we'd have to a) use experimental-mv ABI and b) add For the record, if I set
That's possibly related to #127318. |
Is this with wasm32-unknown-unknown or WASI? Because there's a big difference between those two as well in their C ABI. I believe the unknown target's default C ABI is incompatible with multivalue anyway. |
I would agree with @CryZe that the best solution is to basically "do nothing" and disable any tests related to multivalue. (and/or just comment out the multivalue parts of the tests). There's been various discussions in a bunch of places with varying degrees of depth and complexity but the basic conclusion, in my opinion, is that multivalue just isn't feasible with LLVM. The The goal at this point is to match Clang's C ABI on all targets, whether or not they're |
@alexcrichton To check my understanding here, we have following ABIs:
Is that correct? In particular I want to confirm that 2 and 3 are not the same thing. The suggestion here is that we can go ahead and remove If that's the case, I'd rather just go ahead and do that rather than silently break it on LLVM 19. |
I believe that's correct yeah, and I'll cc @daxpedda here as well since they added Put another way the only way to keep |
Thank you for pinging me! Removing However, I believe we should not enable the multivalue and reference types target features by default, as both are unimplemented and it would be wrong for us to advertise that they are required to read the Wasm module. I'm unsure exactly how to do that, but I believe we might want to patch our fork here and here or use the Or maybe we might consider stabilizing the multivalue feature without the ability to export functions using it. This would still be useful for non-exported functions. But probably not because its still exhibiting bugs. |
Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked in rust-lang#83788). As discussed in rust-lang#127513 (comment) and following, this ABI is a failed experiment that did not end up being used for anything. Keeping support for this ABI in LLVM 19 would require us to switch wasm targets to the `experimental-mv` ABI, which we do not want to do. It should be noted that `Abi::Wasm` was internally used for two things: The `-Z wasm-c-abi=legacy` ABI that is still used by default on some wasm targets, and the `extern "wasm"` ABI. Despite both being `Abi::Wasm` internally, they were not the same. An explicit `extern "wasm"` additionally enabled the `+multivalue` feature. I've opted to remove `Abi::Wasm` in this patch entirely, instead of keeping it as an ABI with only internal usage. Both `-Z wasm-c-abi` variants are now treated as part of the normal C ABI, just with different different treatment in adjust_for_foreign_abi.
PR for removing
We cannot patch our LLVM fork to make functional changes. We can either use the |
Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked in rust-lang#83788). As discussed in rust-lang#127513 (comment) and following, this ABI is a failed experiment that did not end up being used for anything. Keeping support for this ABI in LLVM 19 would require us to switch wasm targets to the `experimental-mv` ABI, which we do not want to do. It should be noted that `Abi::Wasm` was internally used for two things: The `-Z wasm-c-abi=legacy` ABI that is still used by default on some wasm targets, and the `extern "wasm"` ABI. Despite both being `Abi::Wasm` internally, they were not the same. An explicit `extern "wasm"` additionally enabled the `+multivalue` feature. I've opted to remove `Abi::Wasm` in this patch entirely, instead of keeping it as an ABI with only internal usage. Both `-Z wasm-c-abi` variants are now treated as part of the normal C ABI, just with different different treatment in adjust_for_foreign_abi.
Ah yeah I'll highlight @CryZe's point above in that |
Remove extern "wasm" ABI Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked in rust-lang#83788). As discussed in rust-lang#127513 (comment) and following, this ABI is a failed experiment that did not end up being used for anything. Keeping support for this ABI in LLVM 19 would require us to switch wasm targets to the `experimental-mv` ABI, which we do not want to do. It should be noted that `Abi::Wasm` was internally used for two things: The `-Z wasm-c-abi=legacy` ABI that is still used by default on some wasm targets, and the `extern "wasm"` ABI. Despite both being `Abi::Wasm` internally, they were not the same. An explicit `extern "wasm"` additionally enabled the `+multivalue` feature. I've opted to remove `Abi::Wasm` in this patch entirely, instead of keeping it as an ABI with only internal usage. Both `-Z wasm-c-abi` variants are now treated as part of the normal C ABI, just with different different treatment in adjust_for_foreign_abi.
Rollup merge of rust-lang#127605 - nikic:remove-extern-wasm, r=oli-obk Remove extern "wasm" ABI Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked in rust-lang#83788). As discussed in rust-lang#127513 (comment) and following, this ABI is a failed experiment that did not end up being used for anything. Keeping support for this ABI in LLVM 19 would require us to switch wasm targets to the `experimental-mv` ABI, which we do not want to do. It should be noted that `Abi::Wasm` was internally used for two things: The `-Z wasm-c-abi=legacy` ABI that is still used by default on some wasm targets, and the `extern "wasm"` ABI. Despite both being `Abi::Wasm` internally, they were not the same. An explicit `extern "wasm"` additionally enabled the `+multivalue` feature. I've opted to remove `Abi::Wasm` in this patch entirely, instead of keeping it as an ABI with only internal usage. Both `-Z wasm-c-abi` variants are now treated as part of the normal C ABI, just with different different treatment in adjust_for_foreign_abi.
Finished benchmarking commit (0b5eb7b): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -4.6%, secondary -2.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -5.3%, secondary -4.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.5%, secondary 1.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 769.779s -> 755.634s (-1.84%) |
Improvements far outweigh the regressions. @rustbot label: +perf-regression-triaged |
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization. r? `@nikic`
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization. r? `@nikic`
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization. try-job: i686-gnu
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang#127513 closes rust-lang#50156 cc rust-lang#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang/rust#127513 closes rust-lang/rust#50156 cc rust-lang/rust#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.
Add range attribute to scalar function results and arguments as LLVM 19 adds the range attribute this starts to use it for better optimization. hade been interesting to see a perf run with the rust-lang/rust#127513 closes rust-lang/rust#50156 cc rust-lang/rust#49572 shall be fixed but not possible to see as there is asserts that already trigger the optimization.
Loongarch previously had a selection failure for `f16` math [1]. This was fixed in [2], which Rust got with the update to LLVM 19 [3]. Enable loongarch in `std/build.rs` so we start running tests. [1]: llvm/llvm-project#93894 [2]: llvm/llvm-project#94456 [3]: rust-lang#127513
Enable `f16` tests on loongarch Loongarch previously had a selection failure for `f16` math llvm/llvm-project#93894. This was fixed in llvm/llvm-project#94456, which Rust got with the update to LLVM 19 rust-lang#127513. Enable loongarch in `std/build.rs` so we start running tests. try-job: dist-loongarch64-linux try-job: dist-loongarch64-musl
For the |
FYI @rust-lang/release as this is easy to miss. |
This post is intended to be a summary of the changes and impact to users after discussion in rust-lang/rust#127513, rust-lang/rust#128511, and some surrounding issues.
The LLVM 19.1.0 final release is planned for Sep 3rd. The rustc 1.82 stable release will be on Oct 17th.
The unstable MC/DC coverage support is temporarily broken by this update. It will be restored by #126733. The implementation changed substantially in LLVM 19, and there are no plans to support both the LLVM 18 and LLVM 19 implementation at the same time.
This update enables additional target features by default for wasm targets. See #128511 (comment) for details.
Related changes:
Fixes #121444.
Fixes #128212.