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

Honor RUSTFLAGS's target-cpu if given #75

Merged
merged 1 commit into from
Apr 4, 2020

Conversation

ryoqun
Copy link
Contributor

@ryoqun ryoqun commented Apr 2, 2020

Hey, thanks for maintaining a nice crate! This is a tiny build-related PR.

Problem

CPU instruction detection is done at compile time, which isn't compatible for cross-compiling in some case. For example, there is a condition in which compilation is done by a machine with AVX512 and compiled using it and then the binary is transferred to an older machine which lacks the AVX512 support.

Solution

To fix the cross compile issus, we can make reed-solomon's build.rs to accept/honor given RUSTFLAGS, which might have some info for intended CPU architecture in it.

So, we're forcibly reinterpreting RUSTFLAGS's cpu-type as CFLAGS's -march, but it's safe, I think.

That's because as far as I'm aware, the acceptable value of target-cpu is universal across OSS C and Rust toolchains, because rust and clang share the same backend (LLVM), then LLVM was in turn heavily influenced by the GCC because of the compatibility.

Ideally, these should be runtime-based switching for each SIMD implementations, but it would take some more effort than this.

FYI: we're coming from @solana-labs. I'm a collegue with @sakridge, who contributed as well. :) I've created an identical PR for our forked version of this repo: solana-labs#1

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 96.259% when pulling bca41bb on ryoqun:guess-target-cpu-for-upstream into 195263f on darrenldl:master.

@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #75 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   96.27%   96.27%           
=======================================
  Files          10       10           
  Lines        2929     2929           
=======================================
  Hits         2820     2820           
  Misses        109      109           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 195263f...bca41bb. Read the comment docs.

@darrenldl darrenldl mentioned this pull request Apr 2, 2020
@darrenldl
Copy link
Contributor

Thanks for the PR!

I think the fix is a really nice way of handling that in build.rs (I recall choosing -march=native initially after trial and error and giving up).

However, I'm gonna say that I haven't done much Rust nowadays, and so I'll wait a bit and see if other contributors or users have any more feedback.

@darrenldl darrenldl merged commit e79b1fc into rust-rse:master Apr 4, 2020
@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 5, 2020

@darrenldl Thanks for merging!! Now, I'm looking forward to a new patch release, hopefully. :)

@darrenldl
Copy link
Contributor

@ryoqun Thanks for the PR again. I'll finish the required admin work (filling in changelog) and make a release in next 24 hours.

@darrenldl
Copy link
Contributor

@ryoqun 4.0.2 has been published to crates.io

Let me know if there's any problem - kinda doing it in a rush, sorry if I missed some stuff during publishing.

@ruuda
Copy link

ruuda commented Jan 26, 2022

So, we're forcibly reinterpreting RUSTFLAGS's cpu-type as CFLAGS's -march, but it's safe, I think.

For target-cpu=generic, rustc will accept that, but Clang rejects -march=generic. But this is a minor issue, target-cpu=generic is not the default.

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