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

Allow customizing CPU features #241

Closed
breezewish opened this issue Apr 16, 2024 · 3 comments · Fixed by #243
Closed

Allow customizing CPU features #241

breezewish opened this issue Apr 16, 2024 · 3 comments · Fixed by #243
Labels
enhancement New feature or request

Comments

@breezewish
Copy link
Contributor

breezewish commented Apr 16, 2024

For example, currently for x86_64 targets we set -mcpu=pentium4. However it is usually not an ideal one because of lacking modern instructions like CRC32, SSE4.2, AVX, AVX2. AVX2 is shipped in 2013 so that it is actually not that "modern" and should have a broad adoption.

On the other hand, some users might indeed want the compiled binary to be as much as portable, so setting these cpu features by default might not be a good choice.

My idea is to allow users specifying CPU features when calling zigbuild, like --target-cpu-features=.... When this is specified, we no longer provide default CPU features.

I came up two ways how these CPU features could be passed from cargo-zigbuild zigbuild to cargo-zigbuild zig cc/c++. Not sure whether there are better ways.

Solution 1:

Users specify the target CPU feature via an environment variable, so that in cargo-zigbuild zig cc/c++ command we receives it transparently.

Solution 2:

Users specify the target CPU feature via cargo-zigbuild zigbuild --target-cpu-features. The CPU features are then passed to the cache file with a hashed suffix (like zigcxx-x86_64-unknown-linux-gnu.2.17-<some_hash>.sh). In this way, multiple concurrent zigbuilds will not cause conflicts with each other, and we can keep the file untouched when the cpu features are unchanged (for compatible with #239). Pass by file could be possibly better, as it is more reproduciable than environment variables.

@messense
Copy link
Member

messense commented Apr 16, 2024

currently for x86_64 targets we set -mcpu=pentium4.

This isn't true? We are only using pentium4 for i686 Linux target.

cargo-zigbuild/src/zig.rs

Lines 1017 to 1022 in 402ddac

arch_str @ ("i586" | "i686") => {
let cpu_arg = if arch_str == "i586" {
"-mcpu=pentium"
} else {
"-mcpu=pentium4"
};

I think we should just go with solution 1: parse RUSTFLAGS to get target-cpu and target-features, then translate them to zig -mcpu flag.

@messense messense added the enhancement New feature or request label Apr 16, 2024
@breezewish
Copy link
Contributor Author

currently for x86_64 targets we set -mcpu=pentium4.

This isn't true? We are only using pentium4 for i686 Linux target.

cargo-zigbuild/src/zig.rs

Lines 1017 to 1022 in 402ddac

arch_str @ ("i586" | "i686") => {
let cpu_arg = if arch_str == "i586" {
"-mcpu=pentium"
} else {
"-mcpu=pentium4"
};

I think we should just go with solution 1: parse RUSTFLAGS to get target-cpu and target-features, then translate them to zig -mcpu flag.

Good idea! In this way Rust land and C land uses the same CPU features, looks better.

@breezewish
Copy link
Contributor Author

breezewish commented Apr 16, 2024

After some investigation I'm now worrying about one case:

cargo-zigbuild zig cc as a compiler wrapper, now changes behavior when there are environmental changes (e.g. RUSTFLAGS). This actually conflicts with other compiler wrappers such as ccache, where they expect the compilers (and compiler wrappers) to produce exactly the same output using the same input files.

TL;DR, what will happen is that, for some crates that tries to invoke ccache internally (like rust-rocksdb), when user changes RUSTFLAGS, rebuild will not happen due to ccache not invalidated.

As an alternative, we could parse the RUSTFLAGS (from config files and env variables) when invoking cargo-zigbuild zigbuild, and then passing it explicitly as part of the compiler wrapper. This may be friendly with these compiler wrappers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants