cc-wrapper: fix -mtune= validation, add ARM, add fallbacks#205091
Merged
1 commit merged intomasterfrom Oct 23, 2023
unknown repository
Merged
cc-wrapper: fix -mtune= validation, add ARM, add fallbacks#2050911 commit merged intomasterfrom unknown repository
1 commit merged intomasterfrom
unknown repository
Conversation
13 tasks
Atemu
reviewed
Dec 9, 2022
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
This pull request was closed.
This file contains hidden or 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
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")