-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[aarch64] add target feature outline-atomics #83655
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
☔ The latest upstream changes (presumably #81866) made this pull request unmergeable. Please resolve the merge conflicts. |
93b958b
to
003065a
Compare
Ping. |
1 similar comment
Ping. |
@sebpop This seems reasonable. Can you add a codegen test that ensures a reference to the outlined atomics is actually generated as expected? |
Also, could you enable this for aarch64-unknown-linux-musl as well (and make sure it links correctly)? |
#84764 should help with this. |
003065a
to
a32938b
Compare
Looks great! r=me if it passes CI. @bors delegate+ |
✌️ @sebpop can now approve this pull request |
@bors r+ |
📌 Commit a32938b has been approved by |
…htriplett [aarch64] add target feature outline-atomics Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test
⌛ Testing commit a32938b with merge d7f747914deb61e11a59af5c049f64384c2e4a5c... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
Is there something I can help with to make the CI tester pass? |
@bors r+ |
💡 This pull request was already approved, no need to approve it again.
|
Hm, As a result this latest version is somewhat off in this context, especially because it technically silently ignores user input. I don't anticipate that to be a huge problem but this rectifying check should be kept logically separate instead of being included with the canonicalizing match block, including with a note to why (i.e. "we added this to our target definition before our earliest LLVM supported it"). Alternatively the appropriate tests can acquire min LLVM version notes or the like. |
I was not able to specify in aarch64 target definition that the outline-atomics feature should be added only for llvm >= 12, i.e., something like:
The problem is that llvm_version is not available in compiler/rustc_target. I am reverting these changes to the previous patch that passed CI tests with requiring min_llvm_version 12 for two of the tests. |
79b65db
to
e5ca2ca
Compare
Yes, I think the codegen "target feature" infrastructure needs to be reworked to make this kind of change easier, but I see no reason to put that on your shoulders: I just want the "noisy" failure mode instead of the silent one. |
If rust is configured with llvm < 12 on aarch64-linux it will output the warn message |
Indeed, and if they inputted it manually expecting it to work, but hadn't yet upgraded their LLVM version, silently ignoring it, it would not provide user feedback to upgrade their LLVM version, which I anticipate would be 300% more vexing and harder to diagnose. I expect, at the moment, the cleanest possible solution here that satisfies all goals, which are:
is to:
|
Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test
e5ca2ca
to
0f9f241
Compare
Excellent, thank you. Let's try this again. |
📌 Commit 0f9f241 has been approved by |
…kingjubilee [aarch64] add target feature outline-atomics Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test
…kingjubilee [aarch64] add target feature outline-atomics Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test
…ingjubilee Rollup of 13 pull requests Successful merges: - rust-lang#83655 ([aarch64] add target feature outline-atomics) - rust-lang#87091 (implement advance_(back_)_by on more iterators) - rust-lang#88451 (Fix an ICE caused by type mismatch errors being ignored) - rust-lang#88452 (VecDeque: improve performance for From<[T; N]>) - rust-lang#89400 (Improve wording of `map_or_else` docs) - rust-lang#89407 (Recommend running `cargo clean` in E0514 output) - rust-lang#89443 (Include the length in BTree hashes) - rust-lang#89444 (rustdoc: use slice::contains instead of open-coding it) - rust-lang#89447 (Improve error message for missing angle brackets in `[_]::method`) - rust-lang#89453 (Consistently use 'supertrait'.) - rust-lang#89483 (Practice diagnostic message convention) - rust-lang#89500 (Fix ICE with buffered lint referring to AST node deleted by everybody_loops) - rust-lang#89508 (Stabilize `const_panic`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…nly, r=workingjubilee Restrict the aarch64 outline atomics test to Linux The test was introduced in rust-lang#83655, which enables the `outline-atomics` feature for aarch64-unknown-linux-* but not for any other aarch64 targets. The test did not check for Linux causing test failures on aarch64-apple-darwin. r? `@workingjubilee`
@sebpop This PR broke building for aarch64-unknown-linux using I don't have access to an aarch64 system, so I'm testing on x86_64-unknown-linux-gnu cross-compiling to aarch64-unknown-linux-gnu. I am using
It is not immediately clear to me what the difference is between |
The missing symbols are distributed by llvm in compiler-rt and by gcc in libgcc. |
I just hit this myself. You need to pass |
Enable outline-atomics by default as enabled in clang by the following commit
https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11
Performance improves by several orders of magnitude when using the LSE instructions
instead of the ARMv8.0 compatible load/store exclusive instructions.
Tested on Graviton2 aarch64-linux with
x.py build && x.py install && x.py test