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

Fix cross build bug for aarch64 with simd-accel #100

Merged
merged 2 commits into from
May 28, 2022

Conversation

kings-way
Copy link
Contributor

@kings-way kings-way commented May 22, 2022

This patch has done 3 things:

  1. Fix cross build bug for aarch64 with simd-accel
    The current logic in build.rs #[cfg(target_arch = "x86_64")] will fail when cross build for aarch64.
    As the build.rs will be compiled to a binary on host, the target_arch value will be set to host architecture.
    Instead, we should read target_arch from environment. Reproduce it on a x86_64 linux host:
    cargo build --target=aarch64-unknown-linux-gnu --features=simd-accel -vv

  2. Set -march=armv8-a as default value for aarch64
    I think we should set a default -march value for aarch64, and this has been tested for aarch64-linux and aarch64-darwin (Apple M1).

  3. Enable aarch64 + simd-accel for Android build
    Enable simd-accel feature for android, and maybe we can also do it for iOS?

I don't know much about MacOS and iOS, please correct me if there is any mistake~
releated: #98 #95 #92

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

  1. This is a good catch, thanks!
  2. Is this actually beneficial in any way? I don't see a significant reason to include it unless it is a major boost and definitely doesn't cause any issues for users of this library, you can still force it to whatever you want with an environment variable
  3. Have you seen significant improvement with SIMD version on Android?

@kings-way
Copy link
Contributor Author

kings-way commented May 24, 2022

Hi @nazar-pc, in regard to your 2nd and 3rd questions, I did some tests just now. And the answer is ... no.
These benchmarks ran on Qualcomm Snapdragon 865 running Android 11, with both gcc 10 and gcc 4.9 (from android NDK)

simd-accel -march benchmark
Enabled armv8.2-a 630MB/s
Enabled armv8-a 630MB/s
Enabled none 630MB/s
Disabled - 630MB/s

It seems that the C compiler (gcc here) has already done it's job for aarch64. No need to set -march option.
So I am going to do another force push to my master branch, only do the bug fix.

@nazar-pc
Copy link
Member

Can you also simplify that match to a single line it was before, bump patch version and add a changelog entry similar to existing ones so I can make a new release, please?

@kings-way
Copy link
Contributor Author

Hi, put the match or if statement in one single line brings us nothing but ugly code.
if "x86_64" == env::var("CARGO_CFG_TARGET_ARCH").unwrap().as_str() { build.flag(&"-march=haswell"); }

As for the version and changelog, I have pushed some newer commits.
Let me know if there is anything missed.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@nazar-pc nazar-pc merged commit 9f97491 into rust-rse:master May 28, 2022
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.

2 participants