Skip to content

cc-wrapper: fix -mtune= validation, add ARM, add fallbacks#205091

Merged
1 commit merged intomasterfrom
unknown repository
Oct 23, 2023
Merged

cc-wrapper: fix -mtune= validation, add ARM, add fallbacks#205091
1 commit merged intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Dec 8, 2022

Description of changes

Before this commit, cc-wrapper/default.nix was using isGccArchSupported to validate -mtune= values. This has three problems:

This commit fixes these three problems by adding a new isGccTuneSupported function. For isx86 this returns true for the two special values and otherwise defers to isGccArchSupported. This commit also adds support for -mtune= on Arm64, PowerPC, and MIPS.

On Arm64, Clang does not accept as wide a variety of -mtune= values as Gcc does. In particular, Clang does not tune for big.LITTLE mixed-model chips like the very popular RK3399 (found in 6 of the 33 ARM boards on the NixOS wiki), which is -mtune=cortex-a72.cortex-a53. To address this problem, this commit also adds a function findBestTuneApproximation which can be used to map clang-unsupported tunings like cortex-a72.cortex-a53 to less-precise tunings like cortex-a53.

The work which led to this commit arose because we now have packages, like crosvm, which use both clang and gcc; NIX_CFLAGS_COMPILE can no longer be used for those packages since it doesn't distinguish between clang and gcc.

Things done
  • Built on platform(s)
    • x86_64-linux (full workstation rebuild with gcc.arch="bdver1"; gcc.tune="native")
    • aarch64-linux (with gcc.tune="cortex-a72.cortex-a53")
    • powerpc64le-linux (full workstation rebuild with gcc.tune="power9")
    • mips64el-linux (cross from powerpc64le-linux, router packageset with gcc.tune="octeon3")
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Dec 8, 2022
@ghost ghost marked this pull request as ready for review December 9, 2022 04:38
@ghost ghost requested a review from Ericson2314 as a code owner December 9, 2022 04:38
@ghost ghost marked this pull request as draft April 24, 2023 02:22
@ghost ghost marked this pull request as ready for review June 21, 2023 05:14
@ghost ghost requested a review from Atemu October 22, 2023 07:24
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.

Before this commit, cc-wrapper/default.nix was using
`isGccArchSupported` to validate `-mtune=` values.  This has two
problems:

- On x86, `-mtune=` can take the same values as `-march`, plus two
  additional values `generic` and `intel` which are not valid for
  `-march`.

- On ARM, `-mtune=` does not take the same values as `-march=`;
  instead it takes the same values as `-mcpu`.

This commit fixes these two problems by adding a new
`isGccTuneSupported` function.  For `isx86` this returns `true` for
the two special values and otherwise defers to `isGccArchSupported`.

This commit also adds support for `-mtune=` on Aarch64.

Unfortunately on Aarch64, Clang does not accept as wide a variety of
`-mtune=` values as Gcc does.  In particular, Clang does not tune
for big.LITTLE mixed-model chips like the very popular RK3399, which
is targeted using `-march=cortex-a72.cortex-a53` in gcc.

To address this problem, this commit also adds a function
`findBestTuneApproximation` which can be used to map
clang-unsupported tunings like `cortex-a72.cortex-a53` to
less-precise tunings like `cortex-a53`.

The work which led to this commit arose because we now have
packages, like `crosvm`, which use *both* `clang` *and* `gcc`.
Previously I had been using `overrideAttrs` to set
`NIX_CFLAGS_COMPILE` on a package-by-package basis based on which
compiler that package used.  Since we now have packages which use
*both* compilers, this strategy no longer works.

I briefly considered splitting `NIX_CFLAGS_COMPILE` into
`NIX_CFLAGS_COMPILE_GCC` and `NIX_CFLAGS_COMPILE_CLANG`, but since
`NIX_CFLAGS_COMPILE` is sort of a hack to begin with I figured that
adding the logic to `cc-wrapper` would be preferable.
@Atemu Atemu requested a review from Artturin October 22, 2023 10:49
@Atemu Atemu added the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 22, 2023
@ghost ghost merged commit 0b2036c into NixOS:master Oct 23, 2023
@ghost ghost deleted the clang-tune branch October 23, 2023 01:31
This pull request was closed.
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: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant