Skip to content
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

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

sebpop
Copy link
Contributor

@sebpop sebpop commented Mar 29, 2021

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

@rust-highfive
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2021
@bors
Copy link
Contributor

bors commented Apr 3, 2021

☔ The latest upstream changes (presumably #81866) made this pull request unmergeable. Please resolve the merge conflicts.

@sebpop sebpop force-pushed the arm64-outline-atomics branch 3 times, most recently from 93b958b to 003065a Compare April 5, 2021 17:17
@sebpop
Copy link
Contributor Author

sebpop commented Apr 13, 2021

Ping.

1 similar comment
@sebpop
Copy link
Contributor Author

sebpop commented Apr 21, 2021

Ping.

@joshtriplett
Copy link
Member

@sebpop This seems reasonable. Can you add a codegen test that ensures a reference to the outlined atomics is actually generated as expected?

@joshtriplett
Copy link
Member

Also, could you enable this for aarch64-unknown-linux-musl as well (and make sure it links correctly)?

@joshtriplett
Copy link
Member

#84764 should help with this.

@joshtriplett
Copy link
Member

Looks great! r=me if it passes CI.

@bors delegate+

@bors
Copy link
Contributor

bors commented Apr 30, 2021

✌️ @sebpop can now approve this pull request

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 1, 2021

📌 Commit a32938b has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 1, 2021
…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
@bors
Copy link
Contributor

bors commented May 1, 2021

⌛ Testing commit a32938b with merge d7f747914deb61e11a59af5c049f64384c2e4a5c...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 1, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 1, 2021
@sebpop
Copy link
Contributor Author

sebpop commented May 11, 2021

Is there something I can help with to make the CI tester pass?

@sebpop
Copy link
Contributor Author

sebpop commented May 17, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 17, 2021

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: Override clone_from for some types #85176

@workingjubilee
Copy link
Member

workingjubilee commented Sep 29, 2021

Hm, to_llvm_feature is for canonicalizing Rust feature names to LLVM feature names so that user input only has to use one feature name invariantly of codegen backend. However, currently we also expose a pass-through to LLVM-only features. This changeset happens to work here because that function is invariantly over all input, including both user feature input and the compiler's target definition input, and both Rust feature names and LLVM feature names. It including a version check is just because in LLVM v14, LLVM intends to split the definition of a feature in two, so now the official Rust feature means two LLVM features.

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.

@sebpop
Copy link
Contributor Author

sebpop commented Sep 29, 2021

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:

            features: if llvm_version >= 12 {                                                                                                                                                                      
                "+outline-atomics".to_string()                                                                                                                                                                     
            },

The problem is that llvm_version is not available in compiler/rustc_target.
llvm_version only becomes available in compiler/rustc_codegen_llvm/ that gets linked with llvm libs.
That's why I tried to remove outline-atomics feature when the llvm compiler does not support it during normalization of target features.

I am reverting these changes to the previous patch that passed CI tests with requiring min_llvm_version 12 for two of the tests.

@workingjubilee
Copy link
Member

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.

@sebpop
Copy link
Contributor Author

sebpop commented Sep 29, 2021

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 '+outline-atomics' is not a recognized feature for this target (ignoring feature) for all compiles. I am thinking that could be annoying.

@workingjubilee
Copy link
Member

workingjubilee commented Sep 29, 2021

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:

  • no nuisance to existing users who upgrade to the latest nightly but have a pinned LLVM
  • no ignoring explicit user requests silently
  • landing this feature on a "by default" basis

is to:

  • remove it from the target definition
  • do a version / arch check / OS check and then add the feature to aarch64-linux on LLVM 12+
  • add a FIXME comment that it can go in the target definition when our earliest LLVM is 12

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
@workingjubilee
Copy link
Member

Excellent, thank you. Let's try this again.
@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2021

📌 Commit 0f9f241 has been approved by workingjubilee

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…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
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2021
…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
@bors bors merged commit 4e9cf04 into rust-lang:master Oct 4, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 4, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 16, 2021
…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`
@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2021

@sebpop This PR broke building for aarch64-unknown-linux using -Zbuild-std. I was wondering if you or anyone else here can provide some guidance on where to look at how to fix it.

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 aarch64-unknown-linux-gnueabi-gcc as a linker. Running cargo build -Zbuild-std --target aarch64_unknown-linux-gnu on a hello world project yields several linking errors with atomics:

  = note: /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_add::hd35126eded0fd48a':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2392: undefined reference to `__aarch64_ldadd8_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2390: undefined reference to `__aarch64_ldadd8_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2389: undefined reference to `__aarch64_ldadd8_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2391: undefined reference to `__aarch64_ldadd8_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2393: undefined reference to `__aarch64_ldadd8_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_sub::h509cbca9066c2819':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2408: undefined reference to `__aarch64_ldadd8_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2406: undefined reference to `__aarch64_ldadd8_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2405: undefined reference to `__aarch64_ldadd8_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2407: undefined reference to `__aarch64_ldadd8_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2409: undefined reference to `__aarch64_ldadd8_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_sub::hf59db215e96f1bc4':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2408: undefined reference to `__aarch64_ldadd4_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2406: undefined reference to `__aarch64_ldadd4_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2405: undefined reference to `__aarch64_ldadd4_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2407: undefined reference to `__aarch64_ldadd4_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2409: undefined reference to `__aarch64_ldadd4_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_swap::h11448fb64a88a78e':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2376: undefined reference to `__aarch64_swp8_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2374: undefined reference to `__aarch64_swp8_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2373: undefined reference to `__aarch64_swp8_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2375: undefined reference to `__aarch64_swp8_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2377: undefined reference to `__aarch64_swp8_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_swap::h449dcb7a0edc4e35':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2376: undefined reference to `__aarch64_swp4_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2374: undefined reference to `__aarch64_swp4_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2373: undefined reference to `__aarch64_swp4_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2375: undefined reference to `__aarch64_swp4_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2377: undefined reference to `__aarch64_swp4_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_swap::h6031e09dc0173e7a':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2376: undefined reference to `__aarch64_swp1_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2374: undefined reference to `__aarch64_swp1_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2373: undefined reference to `__aarch64_swp1_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2375: undefined reference to `__aarch64_swp1_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2377: undefined reference to `__aarch64_swp1_acq_rel'
          collect2: error: ld returned 1 exit status

It is not immediately clear to me what the difference is between -Zbuild-std and a normal build done by x.py. One theory I have is that -Zbuild-std doesn't use the c feature of compiler-builtins, but I'm uncertain (it's a bit hard to test). Any ideas?

@sebpop
Copy link
Contributor Author

sebpop commented Nov 4, 2021

The missing symbols are distributed by llvm in compiler-rt and by gcc in libgcc.
It looks like your program does not link against one of these two libraries.
As you are cross-compiling, you will need the aarch64 version of compiler-rt or libgcc.

@Amanieu
Copy link
Member

Amanieu commented Nov 23, 2021

One theory I have is that -Zbuild-std doesn't use the c feature of compiler-builtins, but I'm uncertain (it's a bit hard to test). Any ideas?

I just hit this myself. You need to pass -Zbuild-std-features=compiler-builtins-c and then set the COMPILER_RT_ROOT environment variable. It's a bit of a hassle so I just disabled outlined atomics with RUSTFLAGS=-Ctarget-feature=-outline-atomics`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.