core::num::f16b Rust's 16bit Brain Float - #160859
Jamesbarford wants to merge 6 commits into
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 👍
There was a problem hiding this comment.
Actually, as @folkertdev pointed out, it was a doddle. So I've included the implementation in the PR 👍
bf10f8a to
01c5c1b
Compare
|
cc @bjorn3 |
This comment has been minimized.
This comment has been minimized.
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 |
d8ba332 to
2f22e82
Compare
This comment has been minimized.
This comment has been minimized.
…lkertdev Make sin, cos, exp, exp2, log, log2, log10 generic Rebased and smaller version of rust-lang/rust#153934 Following `fabs`, make the `sin`, `cos`, `exp`, `exp2`, `log`, `log2` and `log10` intrinsics generic over the float type, rather than having four variants per float type. The first two commits are purely stylistic: - reorganised Cranelift code to make following changes simpler - moved a misplaced comment in `compiler/rustc_codegen_llvm/src/intrinsic.rs` that caused `x fmt` to give up The last commit actually makes them generic! Most code is a bit simpler, ~~and this will also hopefully simplify adding support for these intrinsics for the future [`bf16` type](rust-lang/rust#160859 :) Unfortunately both GCC and Cranelift backend changes are a bit churny. Their code is a bit, opaque, to put it kindly, and I didn't want to refactor those here. r? @folkertdev cc @RalfJung
This comment has been minimized.
This comment has been minimized.
2f22e82 to
a9b2c63
Compare
This comment has been minimized.
This comment has been minimized.
a9b2c63 to
73621dc
Compare
|
@bors try jobs=test-various,aarch64-apple-,-gnu-nopt-,x86_64-mingw-,aarch64-msvc-*,arm-android |
This comment has been minimized.
This comment has been minimized.
`core::num::f16b` Rust's 16bit Brain Float try-job: test-various try-job: aarch64-apple-* try-job: *-gnu-nopt-* try-job: x86_64-mingw-* try-job: aarch64-msvc-* try-job: arm-android
|
💔 Test for 8f68b95 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
I believe some of the jobs were renamed @bors try jobs=test-various,test-aarch64-apple-,-gnu-nopt-,test-x86_64-mingw-,test-aarch64-msvc-*,test-arm-android |
This comment has been minimized.
This comment has been minimized.
`core::num::f16b` Rust's 16bit Brain Float try-job: test-various try-job: test-aarch64-apple-* try-job: *-gnu-nopt-* try-job: test-x86_64-mingw-* try-job: test-aarch64-msvc-* try-job: test-arm-android
| // RISCV64: fmv.x.w a0, fa0 | ||
| // RISCV64-NEXT: lui a1, 1048560 | ||
| // RISCV64-NEXT: or a0, a0, a1 | ||
| // RISCV64-NEXT: fmv.w.x fa0, a0 | ||
| // RISCV64-NEXT: ret |
There was a problem hiding this comment.
It seems to be setting the high bits to 0xffff here, which is kind of weird. I assume the ABI permits garbage in the upper bits (looking at what it does for f16b_to_bits) rather than requiring 0xffff.... for returns but apparently not passing? Seems worth an LLVM optimization issue if there isn't one.
Cc @beetrees I'm sure you have a better idea about this.
There was a problem hiding this comment.
RISC-V uses NaN-boxing for its floating point registers (which f16b is passed/returned in on all hard-float ABIs), so the high bits must be set to all 1s. Without the zfhmin/zfbfmin extensions, the 32-bit move instructions have to be used, meaning the bits have to set to 1 manually. LLVM currently doesn't seem to keep track of whether the high bits have been set or not at the moment, leading to redundant manual NaN-boxing like in this case (as the argument will already be NaN-boxed). It's definitely something LLVM needs to optimise better. This doesn't affect the f16b_to_bits case as the return value is a u16, not a f16b.
There was a problem hiding this comment.
Is the resolution here to create an issue in LLVM for this optimisation? Or is there something I should have done differently with the setup of the tests for RISC-V?
73621dc to
1b5171f
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. |
| // FIXME(#160859) - This should be a `#[rustc_intrinsic]` using LLVM's `fpext` | ||
| // with this implementation constituting the fallback. | ||
| #[inline] | ||
| const fn widen(value: f16b) -> f32 { |
There was a problem hiding this comment.
Nit: this looks to be the number for the PR which means it will link to something closed. Usually FIXMEs link to open issues so we know that when they show up as closed, they can be resolved.
I'd just make it FIXME(f16b) to match our FIXME(f16)/FIXME(f128).
|
All my concerns have been addressed but I'll leave the final review to @folkertdev. The nits commit could be squashed ofc. |
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;