core::num::f16b Rust's 16bit Brain Float - #160859
Conversation
|
Some changes occurred in compiler/rustc_attr_ir cc @jdonszelmann, @JonathanBrouwer This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410
|
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
@rustbot reroll |
| } | ||
|
|
||
| fn type_f16b(&self) -> Type<'gcc> { | ||
| bug!("f16b is not supported by the GCC codegen backend") |
There was a problem hiding this comment.
I believe GCC actually supports this type: https://github.com/rust-lang/gccjit.rs/blob/master/src/context.rs#L1482
There was a problem hiding this comment.
Thanks 😄, I will aim to add it in a follow up PR 👍
bf10f8a to
01c5c1b
Compare
|
cc @bjorn3 |
There was a problem hiding this comment.
What is the calling convention of other targets?
There was a problem hiding this comment.
I believe only these files were changed because they have an exhaustive match on Float.
However, my version of abi-cafe found two interesting failures: GCC and Clang are inconsistent on aarch64 and armv7
// callee, compiled with GCC 12
#include <inttypes.h>
#include <string.h>
#include <stdio.h>
#include <stdbool.h>
typedef struct Many1 {
__bf16 f0;
} Many1;
void struct_in_1(Many1 arg0) {
printf("%d", arg0.f0);
}// caller, compiled with clang 23
#include <inttypes.h>
#include <string.h>
#include <stdio.h>
#include <stdbool.h>
typedef struct Many1 {
__bf16 f0;
} Many1;
void struct_in_1(Many1 arg0);
void do_test(void) {
{
Many1 arg0 = { .f0 = (((union { uint16_t bits; __bf16 value; }){ .bits = 49600 }).value) };
printf("%d", arg0.f0);
struct_in_1(arg0);
}
}hits
func struct_in_1's values differed
values (native-endian hex bytes):
expect: C0 C1
caller: C0 C1
callee: 04 00
the value was arg0.f0: rustarithmeticty(f16b)
whose arg was arg0: Many1
The current Rust implementation matches clang, and is hence incompatible with GCC
armv7 with hardware floats also runs into incompatibilities
[target.armv7-unknown-linux-gnueabihf."f16::conv_c::repr_c::clang-nightly_calls_distro-gcc"]
busted = "check"
[target.armv7-unknown-linux-gnueabihf."f16::conv_c::repr_c::distro-gcc_calls_clang-nightly"]
busted = "check"
[target.armv7-unknown-linux-gnueabihf."f16b::conv_c::repr_c::clang-nightly_calls_distro-gcc"]
busted = "check"
[target.armv7-unknown-linux-gnueabihf."f16b::conv_c::repr_c::distro-gcc_calls_clang-nightly"]
busted = "check"
Finally, you can let this ICE on many targets, e.g. mips, powerpc, s390x, sparc
There was a problem hiding this comment.
You can also (e.g. on loongarch64 https://godbolt.org/z/Y3hdhG4e6) emit a __truncsfbf2 libcall that is not provided (probably needs to be added to compiler-builtins).
I think the ICEs are probably a blocker? That needs a mechanism similar to has_reliable_f128.
There was a problem hiding this comment.
Yes I added it because of the exhaustive match statement in mips64.rs and sparc64.rs I've removed it; 40d3f6e and put in a panic!(...).
With regard to has_reliable_f128, are you envisaging a has_reliable_f16b entry on TargetConfig?
There was a problem hiding this comment.
With regard to
has_reliable_f128, are you envisaging ahas_reliable_f16bentry onTargetConfig?
Exactly
There was a problem hiding this comment.
What's your take on those ABI mismatches? We should track that somewhere.
There was a problem hiding this comment.
I'm not particularly certain what mips64 should do.
It doesn't, at least to my knowledge, have hardware support. Given we aren't implementing scalar arithmetic and an f16b can only be created through a bit pattern or vendor intrinsics, I can't immediately see a practical application? Hence a panic! seems like a pragmatic choice for the time being.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I believe only these files were changed because they have an exhaustive match on Float.
However, my version of abi-cafe found two interesting failures: GCC and Clang are inconsistent on aarch64 and armv7
// callee, compiled with GCC 12
#include <inttypes.h>
#include <string.h>
#include <stdio.h>
#include <stdbool.h>
typedef struct Many1 {
__bf16 f0;
} Many1;
void struct_in_1(Many1 arg0) {
printf("%d", arg0.f0);
}// caller, compiled with clang 23
#include <inttypes.h>
#include <string.h>
#include <stdio.h>
#include <stdbool.h>
typedef struct Many1 {
__bf16 f0;
} Many1;
void struct_in_1(Many1 arg0);
void do_test(void) {
{
Many1 arg0 = { .f0 = (((union { uint16_t bits; __bf16 value; }){ .bits = 49600 }).value) };
printf("%d", arg0.f0);
struct_in_1(arg0);
}
}hits
func struct_in_1's values differed
values (native-endian hex bytes):
expect: C0 C1
caller: C0 C1
callee: 04 00
the value was arg0.f0: rustarithmeticty(f16b)
whose arg was arg0: Many1
The current Rust implementation matches clang, and is hence incompatible with GCC
armv7 with hardware floats also runs into incompatibilities
[target.armv7-unknown-linux-gnueabihf."f16::conv_c::repr_c::clang-nightly_calls_distro-gcc"]
busted = "check"
[target.armv7-unknown-linux-gnueabihf."f16::conv_c::repr_c::distro-gcc_calls_clang-nightly"]
busted = "check"
[target.armv7-unknown-linux-gnueabihf."f16b::conv_c::repr_c::clang-nightly_calls_distro-gcc"]
busted = "check"
[target.armv7-unknown-linux-gnueabihf."f16b::conv_c::repr_c::distro-gcc_calls_clang-nightly"]
busted = "check"
Finally, you can let this ICE on many targets, e.g. mips, powerpc, s390x, sparc
| }, | ||
| Primitive::Float(float) => match float { | ||
| Float::F16 | Float::F32 => "f32", | ||
| Float::F16 | Float::F16B | Float::F32 => "f32", |
There was a problem hiding this comment.
is that right? LLVM just crashes on bf16 right now, so it's probably at least untested?
There was a problem hiding this comment.
I could be mistaken, however I don't think WASM supports bf16? I've made it panic!(...) for now; 40d3f6e
53d3660 to
40d3f6e
Compare
This comment has been minimized.
This comment has been minimized.
da620d0 to
0ed984f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in cfg and check-cfg configuration cc @Urgau
cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
|
I'm not a good reviewer for this change. Can somebody here pick it up rather than blindly rerolling? |
|
r? me |
|
Reminder, once the PR becomes ready for a review, use |
| // Compiler builtins should have the required conversions to widen to | ||
| // an `f32` if need be. | ||
| _ => true, | ||
| (Arch::AArch64, _) => major >= 19, |
There was a problem hiding this comment.
We only support LLVM 21..=23 right now, and will soon drop support for LLVM 21. So, comparisons with versions lower than that are not meaningful, and when we upgrade comparisons with LLVM 21 will be removed. You can remove those comparisons and just return true.
Independently it would be useful to document here what the criterion is for when support is "reliable", which I think should be two things:
- LLVM should be able to compile fb16 to f32
- the ABI explicitly supports bf16 in its official docs
Can you document the ABI support in the tracking issue?
There was a problem hiding this comment.
I've done Arm, x86_64 and RISC-V; #160630 (comment)
There was a problem hiding this comment.
7938bd4 - commented with a link to the tracking issue, major >= 21 and yes it looks as though arm64ec is something windows specific where ec stands for "Emulation Compatible".
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… and backend handling. GCC and Cranelift explicitly unsupported
…long with traits agreed on in the RFC
…6b` to be treated as a primitive scalar
- Add documentation aliases to `f16b` - Add Wikipedia link to `bfloat16` - Remove `@ only-x86_64` flag from test
- ICE on `sparc64`, `mips64` & WASM
68f6020 to
1f9f414
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
What's your take on those ABI mismatches? We should track that somewhere.
| // This is similar to <https://github.com/llvm/llvm-project/issues/94434>, however | ||
| // does not work until LLVM 23 on Windows. | ||
| (Arch::Arm64EC, _) => major >= 23, | ||
| (Arch::AArch64, _) | (Arch::X86_64, _) | (Arch::RiscV64, _) => major >= 21, |
There was a problem hiding this comment.
based on https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc loongarch also has __bf16.
There was a problem hiding this comment.
62a7f72 Added loongarch64 along with a test, of which I'd hesitate to say the output is correct. I verified it by comparing the output in the ./build... folder to see if it bore a resemblance to what C produced for something similar; https://godbolt.org/z/o17e1e5bc
| //@ edition: 2021 | ||
| //@ only-aarch64 |
There was a problem hiding this comment.
this probably needs -O too
There was a problem hiding this comment.
can you match tests/ui/feature-gates/feature-gate-f16.rs more closely here?
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #161093) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
Implements the RFC: f16b type. Best reviewed commit by commit, happy to split into separate PRs if that is deemed easier to review. However the line count and surface area is, in my opinion, reasonably small.
Adds;
f16balong withbfloatlang item to work with LLVM, GCC is explicitlyunimplemented!(...)f16bfeature gate, page forf16bon libruscdoc and astruct bf16incore::numf16bas a scalar primitive for scalable vectorsIssues;