Unforced avx and avx2 features

[vkomenda]

Problem

AVX and AVX2 features were forced on x86 instead of being enabled when available on the target.

Summary of Changes

Flag checks are run only if the flags are enabled in RUSTFLAGS:

RUSTFLAGS="-C target-cpu=native" cargo build --release --all

or

RUSTFLAGS="-C target-feature=+avx -C target-feature=+avx2" cargo build --release --all

Fixes #10126

Outstanding issues

Scripts and docs should be updated unless there is a better method without using RUSTFLAGS.

[t-nelson]

@vkomenda I'm glad to hear this resolved the issue you observed. Thanks for picking it up! 🙏

@mvines any preference on specifying RUSTFLAGS? We could do it globally either; directly in the scripts or, by adding a .cargo/config to the project root. Alternatively, I think a .cargo/config in the core directory would just apply there. Either way, I suspect we're going to be enabling some dependency SIMD, so need to consider those side-effects

[mvines]

Set RUSTFLAGS="-C target-cpu=native" within scripts/cargo-install-all.sh?

[vkomenda]

@mvines, is it possible that that script is used for cross-compilation? I'm asking because target-cpu=native or march=native won't work then. I sent a related PR solana-labs/reed-solomon-erasure#2. Otherwise, sure, I can add the flag in that script.

[mvines]

I don't think we have a formal cross-compilation workflow. It seems like your rpi container work is likely taking the lead on that front right now, what works best for you? I'm mostly neutral on this issue overall right now.

[vkomenda]

Hopefully this works as expected. The only problem now is how to build it locally. I used to cargo build --release. However now this also requires setting the correct RUSTFLAGS... Can the target CPU be computed in build.rs as an alternative?

[t-nelson]

@vkomenda yes that should work just fine! You can build by running the script directly, scripts/cargo-install-all.sh. I was working with t-nelson@80c15f1 at the end of last week and confirmed that the expected features were built-in in all cases.

@mvines I'm assuming we also want CI running this configuration, since that's how releases will be configured

[mvines]

Hopefully this works as expected. The only problem now is how to build it locally. I used to cargo build --release. However now this also requires setting the correct RUSTFLAGS... Can the target CPU be computed in build.rs as an alternative?

Wait, target-cpu=native is not the default? Having users need to set RUSTFLAGS before a normal cargo build --release to get binaries for their machine is going to be a footgun. I was under the impression that we'd only need to fiddle with RUSTFLAGS in scripts/cargo-install-all.sh (the script that ultimately produces the release binaries), so that even though we build on machines with AVX2 the binaries would cleanly exit if run on a machine without AVX2

[vkomenda]

Wait, target-cpu=native is not the default?

Correct. Earlier I tried compiling with the empty RUSTFLAGS and none of AVX or AVX2 were enabled. It was on a machine with AVX and without AVX2.

[mvines]

🤯
Oh then a build.rs solution that sets RUSTFLAGS to include target-cpu=native seems way better, but then with a check for the CI env var in scripts/cargo-install-all.sh that overrides this to declare the exact CPU features we expect the release binaries to have.

[t-nelson]

AFAIK, we can commit a .cargo/config to project root and set RUSTFLAGS there

[mvines]

AFAIK, we can commit a .cargo/config to project root and set RUSTFLAGS there

k, so .cargo/config configures the default in the monorepo to use target-cpu=native then we have a CI-envvar gated override in scripts/cargo-install-all.sh to ensure the release binary artifacts are build with predictable CPU flags regardless of what machine a particular CI build runs on. Does that sounds right?

[t-nelson]

That's the way I understand it working. I'll take it for a test drive to ensure the implementation matches the docs

[t-nelson]

With this commit t-nelson@6d3d399, I was able to confirm the expected behavior. That is, an AVX2-supporting build host always emits AVX2 code, while a non-supporting build host does not emit AVX2 upon cargo build, but does upon scripts/cargo-install-all.sh .... I can look into gating it on a CI_RUSTFLAGS or similar (and unifying where this happens)

[mvines]

nice sounds good

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.

[ryoqun]

some validator is again experiencing odd errors opcode invalid. let's revive this pr in some way?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[ryoqun]

@vkomenda Hi, are you going to work on this in near future? I'd like this to move forward. If not possible, I'm happy to take this over. :)

[vkomenda]

@vkomenda Hi, are you going to work on this in near future? I'd like this to move forward. If not possible, I'm happy to take this over. :)

I'm happy with you taking it over! I can help you of course if needed.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.