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

Document That f16 And f128 Hardware Support is Limited #123892

Merged
merged 4 commits into from
May 5, 2024

Conversation

ultrabear
Copy link
Contributor

This adds a small paragraph to the recently added f16 and f128 types explaining that hardware support may be limited, and that performance may suffer as a result of that.

I mainly wrote this because I felt it may be useful to express in some form; as a launchpoint for readers of the documentation if they have issues with performance.

I tried to word the documentation in a way that doesn't create false assumptions (that f16/f128 is too slow to use, for instance), removing the software implementation part could mislead people to thinking that f16/f128 is only available on some platforms, not all, so I believe it is important to keep in.
"not all major platforms" is specifically said so as to not be redundant, because not all platforms implement many things, but the average rustacean is probably going to be using x86_64 or aarch64 derived ISA's, which is who this documentation is targeted towards.

I'm not sure of the best way to word the documentation, or if it should even be added, but I feel like it may be useful to have (potentially in a reworded way, I'm not very confident in the current wording and cannot decide if that is because it is too vague to be useful or too specific to be generally correct).

@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 13, 2024
@Noratrieb
Copy link
Member

cc @tgross35

@WiSaGaN
Copy link
Contributor

WiSaGaN commented Apr 13, 2024

Would it be helpful to actually state which platform is guaranteed to have support, so that user can have more information for decision making?
The proposed paragraph seems too vague to be helpful.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thanks for updating this! I meant to add something myself at some point but didn't get to it.

Would it be helpful to actually state which platform is guaranteed to have support, so that user can have more information for decision making?
The proposed paragraph seems too vague to be helpful.

I don't think we need to be too specific here since the exact story is pretty fluid, e.g. new simd instructions keep making f16 better. If anything maybe mention that s390x, as well as some versions of PowerPC and RiscV, have hardware f128 acceleration since that it a lot more performance-dependent.

https://hackmd.io/ax9YKCEeSymE4kciy-BcIA#fn15 is where I collected more specific hardware support

library/core/src/primitive_docs.rs Outdated Show resolved Hide resolved
library/core/src/primitive_docs.rs Outdated Show resolved Hide resolved
@tgross35
Copy link
Contributor

@rustbot label +A-docs +F-f16_and_f128

@rustbot rustbot added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` labels Apr 13, 2024
@tgross35
Copy link
Contributor

Those were just rough ideas, you can adjust the exact wording as you see fit.

That being said, LGTM. Feel free to squash.

@workingjubilee
Copy link
Member

workingjubilee commented Apr 14, 2024

@tgross35 SPARC64 XII also has hardware f128, though we don't support SPARC very well.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

It feels deeply incorrect to say "most major platforms" with the Apple M1 in the same room, and with most hardware people actually using having the target features. This is trying to talk about, and thus effectively skirting around, "rustc is a coward and doesn't enable anything but the minimum target features, so on x86-64 Linux this goes back 20 years of hardware advances".

Let us actually be precise.

library/core/src/primitive_docs.rs Show resolved Hide resolved
Comment on lines 1186 to 1187
/// Note that most major platforms do not have hardware support for `f128`, in which case a
/// software implementation will be used. This can be significantly slower than using `f64`.
Copy link
Member

Choose a reason for hiding this comment

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

"Major platforms" feels a bit weasel-wordy to me. Let's be plainer: before 2015, no commodity hardware with f128 support existed, and today, no target rustc supports has it by default, without enabling target features.

Copy link
Contributor Author

@ultrabear ultrabear Apr 15, 2024

Choose a reason for hiding this comment

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

Removed for clarity (no longer relevant) So I'm currently working on rewriting the f128 section with this feedback, but I don't really know how to approach "you need target features for it today" without being wrong in the future, should we insert a timestamp of some form (rustc version, literal date), or accept that this may become a maintenance burden in exchange for expressing these concepts today?

Copy link
Contributor Author

@ultrabear ultrabear Apr 15, 2024

Choose a reason for hiding this comment

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

Removed for clarity (no longer relevant) I have seen no timestamps anywhere in the docs currently, so I don't think that will be the answer regardless, but how much of a potential maintenance burden should be allocated to this? Edit: I realize now that the PR in its original form also has a maintenance burden as platform support evolves, I did forget to consider that when doing my initial writeup, oops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I've decided to just go forwards with potential maintenance burden, since this PR has had that element from the start regardless

Copy link
Contributor

@tgross35 tgross35 Apr 15, 2024

Choose a reason for hiding this comment

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

I think Jubilee was just suggesting say that even on targets that have hardware support, you need to enable relevant target features to make use of it - not necessarily to actually make a list here (though an example would be fine). Some target features are listed at https://doc.rust-lang.org/reference/attributes/codegen.html#the-target_feature-attribute so it is fine to point there, but even that isn't always up to date...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Jubilee was just suggesting say that even on targets that have hardware support, you need to enable relevant target features to make use of it - not necessarily to actually make a list here (though an example would be fine). Some target features are listed at https://doc.rust-lang.org/reference/attributes/codegen.html#the-target_feature-attribute so it is fine to point there, but even that isn't always up to date...

Oh yeah it was never on my mind to actually list them out, just the idea that "you need target features" will probably be outdated someday, but as mentioned its a burden that is carried by this documentation inherently: the real world will change; commenting on it is inherently unstable

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I wouldn't even have considered that a maintenance burden 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have decided to enumerate that riscv has support, amd64 does not, and aarch64 does not, because they fall into the categories of popular archs that most people will use, or being a non mainframe tier processor that has support (riscv), adding the mainframe ISAs that support f128 just feels a bit much, turning the short example list into a more comprehensive looking thing (which it is not intended to be)

Comment on lines 1085 to 1086
/// Note that most major platforms will provide `f16` math support by converting to and from
/// an `f32`, which is usually fairly performant but will not be as fast as using `f32` directly.
Copy link
Member

Choose a reason for hiding this comment

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

We can be more precise here: The biggest reason to use f16, above all else, is for space reasons, when an application doesn't need the full bits for its exponent and precision.

Essentially all new hardware nowadays has native half-precision floating-point instruction support. Even the Raspberry Pi 5. However, that is often less than the minimal ABI-supported levels, so often only half-to-single-to-half-precision floating-point conversion instructions (which do it faster than one might do manually) can be presumed.

So to enable it to go fast requires enabling target features, often.

@ultrabear
Copy link
Contributor Author

ultrabear commented Apr 15, 2024

It feels deeply incorrect to say "most major platforms" with the Apple M1 in the same room, and with most hardware people actually using having the target features. This is trying to talk about, and thus effectively skirting around, "rustc is a coward and doesn't enable anything but the minimum target features, so on x86-64 Linux this goes back 20 years of hardware advances".

Let us actually be precise.

I originally wrote "not all major platforms have hardware support", but that was before the change to talk about f32 downcasting, and lots of things are still building for only mostly recent amd64 (like 5-10 years back now) when avx2 was the latest thing, and avx512 either did not exist or had catastrophic issues with the early generations, the wording can probably use some work if it wasn't clear; most users on amd64 should not be assumed to have avx512

I was intentionally vague with what specifically does support f16/f128, that was mostly to avoid writing false information by accident, and let the reader look into their specific platform; this probably ends up being worse now that I consider it, I agree some actual dates would serve better

@workingjubilee
Copy link
Member

@ultrabear Yes, I understand you used some vagueness to avoid recommending anything overly temporally sensitive. I most certainly think that it is preferred to be more vague than not at all! I just think that some temporal sensitivity is inevitable when commenting on hardware support, so we can accept the hit of having to update the docs eventually someday, in exchange for giving the reader some useful information.

@ultrabear
Copy link
Contributor Author

Finally got back to rewriting the docs, I don't think they are perfect yet but I hope they are significantly improved over the first iterations. I used wikipedia as a source url which I don't think is great? if we would rather link directly to what wikipedia links to there (the riscv specification) it can be changed, I just frankly did not read the riscv specification myself, so I didn't want to include it

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Nice! The f16 doc addition looks much better now. I think I may have wandered into the weeds a bit on my remarks re: f128, sorry about that!

Comment on lines 1190 to 1193
/// Note that no platforms have hardware support for `f128` without enabling target specific features
/// (and [only some consumer level hardware has support][wikipedia-support], for example RISC-V has support, but
/// neither amd64 nor aarch64 has support), in which case a software implementation will be used. This can be
/// significantly slower than using `f64`.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think my prior comments may have been slightly confusing on this point.

I am not aware of hardware implementations for RISCV. It is specified in the ISA spec, and it might be implemented in an FPGA soft-core somewhere, but "just run a custom FPGA core" is synonymous with "no hardware support" for most Rust programmers.

And the RISCV situation is similar to the SPARC64 situation: they specified it long before it was implemented.

Also, I checked and in PowerISA 3.1 they now specify compliancy subsets and quadruple-precision is also optional in most subsets, so yeah, the "base ISA" doesn't include it.

My thought with "major platforms" was that it requires a very subjective definition of major. Basically, I don't think we should try to start arguments with IBM or SiFive. But yes, realistically, people broadly don't have this. I think the important thing to capture here is to inform people that even when an ISA says it has so-and-so, only very specific implementations of that ISA will implement that optional extension.

library/core/src/primitive_docs.rs Outdated Show resolved Hide resolved
library/core/src/primitive_docs.rs Outdated Show resolved Hide resolved
ultrabear added 3 commits May 4, 2024 14:51
Co-authored-by: Trevor Gross <t.gross35@gmail.com>

Update library/core/src/primitive_docs.rs

Co-authored-by: Trevor Gross <t.gross35@gmail.com>

Update library/core/src/primitive_docs.rs
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

Update library/core/src/primitive_docs.rs

Rewrite f16 and f128 hw support comments to match PR feedback

I wrote RISC-V allcaps in all cases, and wrote amd64 lowercase in all
cases, im not sure if either is the more correct way for either
platform, thats just how I normally write them, if theres a precedent
elsewhere it should probably be changed to match though.

Update library/core/src/primitive_docs.rs

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

Update library/core/src/primitive_docs.rs

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

Update library/core/src/primitive_docs.rs
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

Update library/core/src/primitive_docs.rs

Remove orphaned doc link and clean up grammar a bit

Update library/core/src/primitive_docs.rs
@ultrabear ultrabear force-pushed the ultrabear_softfloatdoc branch from 17ea9e3 to 5aa2f9a Compare May 4, 2024 21:52
@workingjubilee
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 4, 2024

📌 Commit 5aa2f9a has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2024
fmease added a commit to fmease/rust that referenced this pull request May 5, 2024
…r=workingjubilee

Document That `f16` And `f128` Hardware Support is Limited

This adds a small paragraph to the recently added f16 and f128 types explaining that hardware support may be limited, and that performance may suffer as a result of that.

I mainly wrote this because I felt it may be useful to express in some form; as a launchpoint for readers of the documentation if they have issues with performance.

I tried to word the documentation in a way that doesn't create false assumptions (that f16/f128 is too slow to use, for instance), removing the software implementation part could mislead people to thinking that f16/f128 is only available on some platforms, not all, so I believe it is important to keep in.\
"not all *major* platforms" is specifically said so as to not be redundant, because not all platforms implement many things, but the average rustacean is probably going to be using x86_64 or aarch64 derived ISA's, which is who this documentation is targeted towards.

I'm not sure of the best way to word the documentation, or if it should even be added, but I feel like it may be useful to have (potentially in a reworded way, I'm not very confident in the current wording and cannot decide if that is because it is too vague to be useful or too specific to be generally correct).
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122253 (Support Result<T, E> across FFI when niche optimization can be used)
 - rust-lang#123892 (Document That `f16` And `f128` Hardware Support is Limited)
 - rust-lang#124458 (Implement lldb formattter for "clang encoded" enums (LLDB 18.1+))
 - rust-lang#124459 (Stabilize exclusive_range_pattern)
 - rust-lang#124711 (Migrate `run-make/doctests-runtool` to rmake)
 - rust-lang#124725 (Meta: Enable the brand new triagebot transfer command)
 - rust-lang#124727 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122253 (Support Result<T, E> across FFI when niche optimization can be used)
 - rust-lang#123892 (Document That `f16` And `f128` Hardware Support is Limited)
 - rust-lang#124458 (Implement lldb formattter for "clang encoded" enums (LLDB 18.1+))
 - rust-lang#124459 (Stabilize exclusive_range_pattern)
 - rust-lang#124711 (Migrate `run-make/doctests-runtool` to rmake)
 - rust-lang#124725 (Meta: Enable the brand new triagebot transfer command)
 - rust-lang#124727 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented May 5, 2024

⌛ Testing commit 5aa2f9a with merge 3fbeedb...

@bors
Copy link
Contributor

bors commented May 5, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 3fbeedb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 5, 2024
@bors bors merged commit 3fbeedb into rust-lang:master May 5, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 5, 2024
@Kobzol
Copy link
Contributor

Kobzol commented May 5, 2024

This PR has been merged by accident, without the full test suite being run. It was my fault (#124631 got merged and it broke our CI). @ultrabear Could you please create a new PR with your branch against the latest version of origin/master? Thank you, and sorry for the mess.

@dtolnay
Copy link
Member

dtolnay commented May 5, 2024

New PR, from the same branch: #124750

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 5, 2024
…r=workingjubilee

Document That `f16` And `f128` Hardware Support is Limited (v2)

This PR is identical to rust-lang#123892, which was approved and merged but then removed from master by a force-push due to a [CI bug](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/ci.20broken.3F).

r? ghost

Original PR description:

---

This adds a small paragraph to the recently added f16 and f128 types explaining that hardware support may be limited, and that performance may suffer as a result of that.

I mainly wrote this because I felt it may be useful to express in some form; as a launchpoint for readers of the documentation if they have issues with performance.

I tried to word the documentation in a way that doesn't create false assumptions (that f16/f128 is too slow to use, for instance), removing the software implementation part could mislead people to thinking that f16/f128 is only available on some platforms, not all, so I believe it is important to keep in.\
"not all *major* platforms" is specifically said so as to not be redundant, because not all platforms implement many things, but the average rustacean is probably going to be using x86_64 or aarch64 derived ISA's, which is who this documentation is targeted towards.

I'm not sure of the best way to word the documentation, or if it should even be added, but I feel like it may be useful to have (potentially in a reworded way, I'm not very confident in the current wording and cannot decide if that is because it is too vague to be useful or too specific to be generally correct).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
Rollup merge of rust-lang#124750 - ultrabear:ultrabear_softfloatdoc, r=workingjubilee

Document That `f16` And `f128` Hardware Support is Limited (v2)

This PR is identical to rust-lang#123892, which was approved and merged but then removed from master by a force-push due to a [CI bug](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/ci.20broken.3F).

r? ghost

Original PR description:

---

This adds a small paragraph to the recently added f16 and f128 types explaining that hardware support may be limited, and that performance may suffer as a result of that.

I mainly wrote this because I felt it may be useful to express in some form; as a launchpoint for readers of the documentation if they have issues with performance.

I tried to word the documentation in a way that doesn't create false assumptions (that f16/f128 is too slow to use, for instance), removing the software implementation part could mislead people to thinking that f16/f128 is only available on some platforms, not all, so I believe it is important to keep in.\
"not all *major* platforms" is specifically said so as to not be redundant, because not all platforms implement many things, but the average rustacean is probably going to be using x86_64 or aarch64 derived ISA's, which is who this documentation is targeted towards.

I'm not sure of the best way to word the documentation, or if it should even be added, but I feel like it may be useful to have (potentially in a reworded way, I'm not very confident in the current wording and cannot decide if that is because it is too vague to be useful or too specific to be generally correct).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants