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

Don't use _Float16 on architectures that do not support it #1182

Merged

Conversation

mcatanzaro
Copy link
Contributor

_Float16 is only supported on a few architectures. Let's assume it's not supported unless we know otherwise.

References:

@mcatanzaro mcatanzaro force-pushed the mcatanzaro/_float16-availability branch from 285b7fd to fff60d6 Compare May 14, 2024 21:11
@mcatanzaro mcatanzaro marked this pull request as draft May 14, 2024 21:13
@mcatanzaro mcatanzaro marked this pull request as ready for review May 14, 2024 21:14
@mcatanzaro mcatanzaro force-pushed the mcatanzaro/_float16-availability branch from fff60d6 to 5c10ad2 Compare May 14, 2024 21:30
@mcatanzaro mcatanzaro marked this pull request as draft May 14, 2024 22:53
@mcatanzaro
Copy link
Contributor Author

So most of the CI results are in and looking good (except for the "default" check, which isn't actionable because there are no build logs available). But I'm marking this as draft because I've realized the current check isn't ideal. With this change, _Float16 will no longer ever be enabled on MIPS or Power. That's good, because it's not supported on those architectures, but now I should simplify the conditionals further. It might also be possible to simplify the RISC-V check.

Basically my general idea here is: only enable _Float16 where we know it actually works. If that's a bad idea, please complain!

@mcatanzaro
Copy link
Contributor Author

mcatanzaro commented May 15, 2024

OK, I'm going to push a new revision that additionally removes the checks for MIPS and POWER, since they should no longer be needed as a result of my changes here. I expect this should be safe.

There are two remaining problems with the checks here, which I don't know how to fix and will not touch:

(1) The checks for RISC-V are strange. The first check semantically means "don't use _Float16 on RISC-V if compiling with Clang" (because it's broken, see 3f4ea16). But the second check enables it for the ZVFH subarchitecture regardless of compiler. This seems odd and should be reconciled somehow.

(2) _Float16 is never used if compiling C++ with GCC on aarch64. But it is used if compiling C, or if compiling with Clang. This seems strange. This exclusion was added in 8910057.

@mcatanzaro mcatanzaro force-pushed the mcatanzaro/_float16-availability branch from 5c10ad2 to defd98c Compare May 15, 2024 14:20
@mcatanzaro mcatanzaro marked this pull request as ready for review May 15, 2024 14:21
mcatanzaro added a commit to mcatanzaro/WebKit that referenced this pull request May 15, 2024
…s not supported on this target

https://bugs.webkit.org/show_bug.cgi?id=274086

Unreviewed build fix for s390x, and also i386 with SSE2 disabled.

I've submitted these changes to upstream SIMDE to ensure they hopefully
don't get lost when rebasing:

simd-everywhere/simde#1182

* Source/WTF/wtf/simde/arm/neon.h:
mcatanzaro added a commit to mcatanzaro/WebKit that referenced this pull request May 15, 2024
…s not supported on this target

https://bugs.webkit.org/show_bug.cgi?id=274086

Unreviewed build fix for s390x, and also i386 with SSE2 disabled.

I've submitted these changes to upstream SIMDE to ensure they hopefully
don't get lost when rebasing:

simd-everywhere/simde#1182

* Source/WTF/wtf/simde/arm/neon.h:
webkit-commit-queue pushed a commit to mcatanzaro/WebKit that referenced this pull request May 15, 2024
…s not supported on this target

https://bugs.webkit.org/show_bug.cgi?id=274086

Unreviewed build fix for s390x, and also i386 with SSE2 disabled.

I've submitted these changes to upstream SIMDE to ensure they hopefully
don't get lost when rebasing:

simd-everywhere/simde#1182

* Source/WTF/wtf/simde/arm/neon.h:

Canonical link: https://commits.webkit.org/278810@main
webkit-commit-queue pushed a commit to mcatanzaro/WebKit that referenced this pull request May 15, 2024
…s not supported on this target

https://bugs.webkit.org/show_bug.cgi?id=274086

Unreviewed build fix for s390x, and also i386 with SSE2 disabled.

I've submitted these changes to upstream SIMDE to ensure they hopefully
don't get lost when rebasing:

simd-everywhere/simde#1182

* Source/WTF/wtf/simde/arm/neon.h:

Canonical link: https://commits.webkit.org/278821@main
@mr-c
Copy link
Collaborator

mr-c commented May 22, 2024

Thank you for looking into this, @mcatanzaro !

With this change, _Float16 will no longer ever be enabled on MIPS or Power. That's good, because it's not supported on those architectures,

What is the evidence that GCC doesn't support float16 on MIPS nor Power?

https://gcc.gnu.org/onlinedocs/gcc/Half-Precision.html says

In cases where hardware support is not specified, GCC implements conversions between __fp16 and other types as library calls.

--

(1) The checks for RISC-V are strange. The first check semantically means "don't use _Float16 on RISC-V if compiling with Clang" (because it's broken, see 3f4ea16). But the second check enables it for the ZVFH subarchitecture regardless of compiler. This seems odd and should be reconciled somehow.

@eric900115 , do you have evidence that _Float16 works on clang for RISC-V if __riscv_zvfh is defined?

(2) _Float16 is never used if compiling C++ with GCC on aarch64. But it is used if compiling C, or if compiling with Clang. This seems strange. This exclusion was added in 8910057.

Perhaps there is a minimum version of g++ that works with _Float16 on aarch64; can you do some testing and find out further?

In general, I prefer to exclude known-broken compiler version + architecture combinations, as documentation of minimum versions and then rely on bug reports where new discoveries are made; rather than only enable native fp16 where we have testing which would mean that users would miss out on the fp16 support in situations we didn't test for.

@mcatanzaro
Copy link
Contributor Author

What is the evidence that GCC doesn't support float16 on MIPS nor Power?

It's on the previous page:

"The _Float16 type is supported on AArch64 systems by default, on ARM systems when the IEEE format for 16-bit floating-point types is selected with -mfp16-format=ieee and, for both C and C++, on x86 systems with SSE2 enabled. "

So it's not available by default, and the allowlisted architectures are just the ones listed. If you prefer I can go back to the denylist approach and just exclude s390x, but you'll probably keep receiving patches to exclude other architectures.

Perhaps there is a minimum version of g++ that works with _Float16 on aarch64; can you do some testing and find out further?

Hm, that sounds like a lot of work, sorry. I'm sure I could emulate aarch64 with qemu, but I've never done that before.

@mr-c
Copy link
Collaborator

mr-c commented May 22, 2024

What is the evidence that GCC doesn't support float16 on MIPS nor Power?

It's on the previous page:

"The _Float16 type is supported on AArch64 systems by default, on ARM systems when the IEEE format for 16-bit floating-point types is selected with -mfp16-format=ieee and, for both C and C++, on x86 systems with SSE2 enabled. "

I think that page is out of date. For example, RISC-V has supported _Float16 since GCC 13
gcc-mirror/gcc@27d68a6 (a.k.a https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=27d68a60783b52504a08503d3fe12054de104241 )

So it's not available by default, and the allowlisted architectures are just the ones listed. If you prefer I can go back to the denylist approach and just exclude s390x, but you'll probably keep receiving patches to exclude other architectures.

I prefer the denylist, yes. Happy to get more patches. Users can workaround this without a patch by defining SIMDE_FLOAT16_API with the appropriate value before including any SIMDe header.

Perhaps there is a minimum version of g++ that works with _Float16 on aarch64; can you do some testing and find out further?

Hm, that sounds like a lot of work, sorry. I'm sure I could emulate aarch64 with qemu, but I've never done that before.

https://godbolt.org/ has a variety of arm64 gcc versions

@mcatanzaro mcatanzaro force-pushed the mcatanzaro/_float16-availability branch from defd98c to fa1984b Compare May 22, 2024 13:08
@mcatanzaro
Copy link
Contributor Author

OK, here's a new version that just excludes s390x and i386. (The disadvantage is this makes the condition even more complex.)

https://godbolt.org/ has a variety of arm64 gcc versions

Huh, godbolt is really good.

Seems the check was actually correct. _Float16 is supported in C since GCC 7 and in C++ only since GCC 13. That's odd, but OK. I'll add another commit to make it available if using GCC 13 or newer.

It seems GCC supports _Float16 in C++ on aarch64 since GCC 13, at least
according to my quick tests on godbolt.org.

(It was already supported in C since GCC 7.)
Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thank you!

@mr-c mr-c enabled auto-merge (rebase) May 22, 2024 14:42
@mr-c mr-c disabled auto-merge May 23, 2024 09:23
@mr-c mr-c merged commit e30e6ec into simd-everywhere:master May 23, 2024
88 of 89 checks passed
@mcatanzaro mcatanzaro deleted the mcatanzaro/_float16-availability branch May 23, 2024 12:40
mcatanzaro added a commit to mcatanzaro/WebKit that referenced this pull request May 23, 2024
https://bugs.webkit.org/show_bug.cgi?id=274581

Reviewed by NOBODY (OOPS!).

The simde build fix landed in 278821@main does not match what I landed
upstream in simd-everywhere/simde#1182. Let's
change our copy to match upstream to flush out any potential problems
sooner rather than later.

* Source/WTF/wtf/simde/arm/neon.h:
webkit-commit-queue pushed a commit to mcatanzaro/WebKit that referenced this pull request May 24, 2024
https://bugs.webkit.org/show_bug.cgi?id=274581

Reviewed by Yusuke Suzuki.

The simde build fix landed in 278821@main does not match what I landed
upstream in simd-everywhere/simde#1182. Let's
change our copy to match upstream to flush out any potential problems
sooner rather than later.

* Source/WTF/wtf/simde/arm/neon.h:

Canonical link: https://commits.webkit.org/279270@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants