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

treewide: remove -march=native #202687

Merged
merged 3 commits into from
Nov 24, 2022
Merged

treewide: remove -march=native #202687

merged 3 commits into from
Nov 24, 2022

Conversation

thiagokokada
Copy link
Contributor

Description of changes

-march=native breaks purity, so let's remove it.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@PedroHLC
Copy link
Member

-march=native breaks purity, so let's remove it.

Not only that, the generated code will expect everybody that downloads it from caches to have the same processor as the builder, we don't use server's CPUs, so probably it makes performance even worse.

@thiagokokada
Copy link
Contributor Author

-march=native breaks purity, so let's remove it.

Not only that, the generated code will expect everybody that downloads it from caches to have the same processor as the builder, we don't use server's CPUs, so probably it makes performance even worse.

From a performance stand point -march=native may increase performance even considering that Hydra != host machine, because we use very conservative options right now: https://github.com/NixOS/nixpkgs/pull/202526/files#diff-e32d2e727a496a1da28a670f938cf5b76322be57c4f86b1d5a6f29eb2c5dead6

But of course, it is impossible to know since it would need tests.

@ofborg ofborg bot requested review from zaninime and eadwu November 24, 2022 15:10
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 24, 2022
@NickCao
Copy link
Member

NickCao commented Nov 24, 2022

These impure flags should have been handled by the cc-wrapper:

# Clear march/mtune=native -- they bring impurity.
if [ "$NIX_ENFORCE_NO_NATIVE_@suffixSalt@" = 1 ]; then
kept=()
# Old bash empty array hack
for p in ${params+"${params[@]}"}; do
if [[ "$p" = -m*=native ]]; then
skip "$p"
else
kept+=("$p")
fi
done
# Old bash empty array hack
params=(${kept+"${kept[@]}"})
fi

@thiagokokada
Copy link
Contributor Author

These impure flags should have been handled by the cc-wrapper:

# Clear march/mtune=native -- they bring impurity.
if [ "$NIX_ENFORCE_NO_NATIVE_@suffixSalt@" = 1 ]; then
kept=()
# Old bash empty array hack
for p in ${params+"${params[@]}"}; do
if [[ "$p" = -m*=native ]]; then
skip "$p"
else
kept+=("$p")
fi
done
# Old bash empty array hack
params=(${kept+"${kept[@]}"})
fi

Nice to know, but I think it is still worth to remove those just to make sure that nobody will follow the same.

@PedroHLC
Copy link
Member

Result of nixpkgs-review pr 202687 run on x86_64-linux 1

2 packages built:
  • glava
  • opentrack

@thiagokokada thiagokokada merged commit 47533b8 into NixOS:master Nov 24, 2022
@thiagokokada thiagokokada deleted the remove-march-native branch November 24, 2022 15:23
@github-actions
Copy link
Contributor

Successfully created backport PR #202693 for release-22.11.

@zhaofengli
Copy link
Member

These impure flags should have been handled by the cc-wrapper

I wasn't aware of this behavior until it was mentioned on Matrix today. It affects usage in an impure mkShell environment as well, which can be surprising/undesirable. I opened #203675 so we can discuss about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants