Skip to content

Implement prefetch hints for aarch64 #918

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

Merged
merged 6 commits into from
Sep 26, 2020
Merged

Implement prefetch hints for aarch64 #918

merged 6 commits into from
Sep 26, 2020

Conversation

dgbo
Copy link
Contributor

@dgbo dgbo commented Sep 24, 2020

Co-authored-by: Wang Maozhang [email protected]

This implements prefetch hints for aarch64 via LLVMs prefetch instrinsic.

We wintness ~20% performance gain in seal_pre_commit_phase1 of Filecoin with 32GB data input.

Verified with cargo +nightly test and cargo +nightly test --release.
There are four failures (as shown below), but these are not cause by this PR.
These tests failed both with and without this PR.

---- core_arch::aarch64::tme::assert___tcommit_tcommit stdout ----
disassembly for stdarch_test_shim___tcommit_tcommit:
         0: adrp x8, 16f000 <_GLOBAL_OFFSET_TABLE_+0x280>
         1: ldr x8, [x8, #3792]
         2: adrp x9, f1000 <anon.1626de38755f4729e9f2a08512645c0a.14.llvm.15569108828795629504+0x10b87>
         3: add x9, x9, #0xd29
         4: str x9, [x8]
         5: msr s0_3_c3_c0_3, xzr
         6: ret
thread 'core_arch::aarch64::tme::assert___tcommit_tcommit' panicked at 'failed to find instruction `tcommit` in the disassembly', crates/stdarch-test/src/lib.rs:144:9


failures:
    core_arch::aarch64::tme::assert___tcancel_tcancel
    core_arch::aarch64::tme::assert___tcommit_tcommit
    core_arch::aarch64::tme::assert___tstart_tstart
    core_arch::aarch64::tme::assert___ttest_ttest

Thanks.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (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.

@dgbo
Copy link
Contributor Author

dgbo commented Sep 24, 2020

The check error is strange:

error[E0723]: can only call `transmute` from const items, not `const fn`
   --> crates/core_arch/src/wasm32/simd128.rs:420:5
    |
420 |     transmute(f64x2(a0, a1))
    |     ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
    = help: add `#![feature(const_fn)]` to the crate attributes to enable

This PR does not touch any code in wasm32 at all.
And the #![feature(const_fn)] is in crates/core_arch/src/lib.rs already.

Could anyone give some suggestion here?

Best Regards.

#[cfg_attr(test, assert_instr("prfm pstl3keep", rw = _PREFETCH_WRITE, locality = _PREFETCH_LOCALITY1))]
#[cfg_attr(test, assert_instr("prfm pstl2keep", rw = _PREFETCH_WRITE, locality = _PREFETCH_LOCALITY2))]
#[cfg_attr(test, assert_instr("prfm pstl1keep", rw = _PREFETCH_WRITE, locality = _PREFETCH_LOCALITY3))]
pub unsafe fn _prefetch(p: *const i8, rw: i32, locality: i32) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use #[rustc_args_required_const(1, 2)] to ensure that rw and locality are compile-time constants.

Copy link
Member

@bjorn3 bjorn3 Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to match on every valid (rw, locality) pair so that the prefetch call always gets constant values as arguments and you need to handle invalid values.

Copy link
Contributor Author

@dgbo dgbo Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the quick review.

The #[rustc_args_required_const(1, 2)] is added just before the function _prefetch.

Created a macro pref to match all valid (rw, locality) pairs.
An invalid pair will cause panic now.
Suggestions?

@Amanieu
Copy link
Member

Amanieu commented Sep 24, 2020

The wasm failure seems to be due to an issue on the latest Rust nightly. You can fix it by adding the const_fn_transmute feature to the list in lib.rs.

@dgbo
Copy link
Contributor Author

dgbo commented Sep 25, 2020

The wasm failure seems to be due to an issue on the latest Rust nightly. You can fix it by adding the const_fn_transmute feature to the list in lib.rs.

Added the const_fn_transmute feature to the list in lib.rs. The CI is green. Thanks.

@Amanieu Amanieu merged commit b422b01 into rust-lang:master Sep 26, 2020
@Amanieu
Copy link
Member

Amanieu commented Sep 26, 2020

Thanks!

bors referenced this pull request in rust-lang-ci/rust Sep 27, 2020
update stdarch submodule

This commit update the src/stdarch submodule, we primarily want to include [https://github.com/rust-lang/stdarch/pull/918](url) which provides prefetch hints for aarch64. This PR could deliver ~20% performance gain on our aarch64 server in Filecoin. Wish this could be used as soon as possible.

Thanks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants