Fix soft float support, split musl triples by float ABI, and enable CI #21269
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
alexrp
changed the title
alexrp
commented
Aug 31, 2024
| @@ -1442,19 +1442,22 @@ test "store vector with memset" { | |||
| if (builtin.zig_backend == .stage2_riscv64) return error.SkipZigTest; | |||
|
|
|||
| if (builtin.zig_backend == .stage2_llvm) { | |||
| // LLVM 16 ERROR: "Converting bits to bytes lost precision" | |||
There was a problem hiding this comment.
Note that this test is being re-enabled for all targets in #21183.
|
Just to give an idea of the direction I'm working towards, the next PR will include:
|
|
Seems like the CI impact here is within reason. |
…nux-musleabi(hf)`. Closes ziglang#21184.
Also rename Target.getFloatAbi() to floatAbi().
Prefer `eabi` and `eabihf` over the ambiguous `none` when not using libc.
Broadly speaking, versions 6, 7, and 8 are the ones that are in common use. Of these, v7 is what you'll typically see for 32-bit Arm today. So let's actually make sure that that's what we're testing.
Similar to __truncsfhf2() and __extendhfsf2().
andrewrk
reviewed
Sep 12, 2024
Comment on lines
+3112
to
+3125
| // `use-soft-float` means "use software routines for floating point computations". In | ||
| // other words, it configures how LLVM lowers basic float instructions like `fcmp`, | ||
| // `fadd`, etc. The float calling convention is configured on `TargetMachine` and is | ||
| // mostly an orthogonal concept, although obviously we do need hardware float operations | ||
| // to actually be able to pass float values in float registers. | ||
| // | ||
| // Ideally, we would support something akin to the `-mfloat-abi=softfp` option that GCC | ||
| // and Clang support for Arm32 and CSKY. We don't currently expose such an option in | ||
| // Zig, and using CPU features as the source of truth for this makes for a miserable | ||
| // user experience since people expect e.g. `arm-linux-gnueabi` to mean full soft float | ||
| // unless the compiler has explicitly been told otherwise. (And note that our baseline | ||
| // CPU models almost all include FPU features!) | ||
| // | ||
| // Revisit this at some point. |
There was a problem hiding this comment.
What makes sense to me is:
- float ABI in the target triple
- whether hard float instructions can be used in the CPU featureset
Furthermore, baseline for soft float ABI target triples can default to not using float instructions in the CPU features.
it's unclear to me if this comment is in agreement with this or not.
|
Thanks for working on this. So far I'm unconvinced that Zig needs another option for controlling the ABI or CPU features. Based on your description it seems to me like this use case can be fully addressed by adjusting the baseline CPU featureset that is selected for soft float ABI targets. |
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.
This is the first round of soft float work that I'm doing.
There are 3 goals of this PR:
I'm completely ignoring
-mfloat-abi=softfpsupport (soft float ABI with FPU computations) and in fact intentionally suppressing it in the LLVM backend as a result of theuse-soft-floatchange. The only thing that matters for this first patch set is correctness; the next round of patches will re-enable support for this optimization in a way that actually functions correctly.Note that a core premise of my work in this area is that, if the user specifies a soft float target triple, the default assumption should be that no FPU code is generated, even if the target CPU model happens to have FPU features. There's a very strong established precedent for this in the C/C++ world where you have to explicitly opt into this behavior by using
-mfloat-abi=softfpinstead of-mfloat-abi=soft(the latter being implied by the soft float triple). But even without that, everyone understands e.g.arm-linux-gnueabito mean soft float, and it's just astonishing as a user if the resulting binary fromzig build -Dtarget=arm-linux-gnueabihas FPU instructions in it and can't run on a fully soft float system.I initially thought that adding soft float targets to CI wasn't necessary, but the state of our soft float support was broken enough that I've changed my mind on this, so this PR also includes those test additions. Let's see if it causes a noticeable slowdown, and if so, we can maybe limit it to Arm or something. (As usual, the MIPS O32 targets are gated by
-Dtest-slow-targetswhich is off by default both locally and in CI.) This branch passeszig build test -fqemu -fwasmtime --glibc-runtimes <...> -Dtest-slow-targetslocally for me with all the added targets. Finally, I also changed the Arm target triples for tests to use v7 instead of v8 because a) v7 is much more common for 32-bit Arm, and b) v8 adds floating point features that don't exist in v7 so things were broken for v7 and it went unnoticed.Closes #10961.
Closes #21184.