cc-wrapper: fix -mtune= validation, add ARM, add fallbacks #205091
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
mentioned this pull request
13 tasks
ofborg
bot
added
10.rebuild-darwin: 0
ghost
marked this pull request as ready for review
Atemu
reviewed
Dec 9, 2022
ghost
marked this pull request as draft
ghost
marked this pull request as ready for review
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
approved these changes
Oct 22, 2023
Atemu
added
the
12.approvals: 1
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Before this commit, cc-wrapper/default.nix was using
isGccArchSupportedto validate-mtune=values. This has three problems:On x86,
-mtune=can take the same values as-march, plus two additional valuesgenericandintelwhich are not valid for-march.On Arm64,
-mtune=does not take the same values as-march=; instead it takes the same values as-mcpu.On PowerPC, all
-march=flags are rejected, soisGccArchSupportedought to unconditionally return false (see cc-wrapper: -march= is not allowed on powerpc #205242), yet-mtuneis allowed.This commit fixes these three problems by adding a new
isGccTuneSupportedfunction. Forisx86this returnstruefor the two special values and otherwise defers toisGccArchSupported. 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 functionfindBestTuneApproximationwhich can be used to map clang-unsupported tunings likecortex-a72.cortex-a53to less-precise tunings likecortex-a53.The work which led to this commit arose because we now have packages, like
crosvm, which use bothclangandgcc;NIX_CFLAGS_COMPILEcan no longer be used for those packages since it doesn't distinguish between clang and gcc.Things done
gcc.arch="bdver1"; gcc.tune="native")gcc.tune="cortex-a72.cortex-a53")gcc.tune="power9")gcc.tune="octeon3")