Skip to content

Use f128 internally for Duration-float methods#154107

Open
cuviper wants to merge 3 commits intorust-lang:mainfrom
cuviper:duration-f128
Open

Use f128 internally for Duration-float methods#154107
cuviper wants to merge 3 commits intorust-lang:mainfrom
cuviper:duration-f128

Conversation

@cuviper
Copy link
Member

@cuviper cuviper commented Mar 19, 2026

If we want to keep as much Duration precision as possible when
multiplying or dividing by a float, then we shouldn't convert it to
f32 nor f64 to match the operand, because their mantissas aren't
large enough to hold a full Duration. However, f128 is large enough
with MANTISSA_DIGITS = 113, since Duration::MAX only needs 94 bits.

Fixes #149794, as an alternative to #150933.

If we want to keep as much `Duration` precision as possible when
multiplying or dividing by a float, then we shouldn't convert it to
`f32` nor `f64` to match the operand, because their mantissas aren't
large enough to hold a full `Duration`. However, `f128` is large enough
with `MANTISSA_DIGITS = 113`, since `Duration::MAX` only needs 94 bits.
@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 Mar 19, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 19, 2026

r? @scottmcm

rustbot has assigned @scottmcm.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, joboet, scottmcm

@tgross35
Copy link
Contributor

I think this is a good solution but we can't actually do this unconditionally now, f128 use still needs to be gated behind cfg(target_has_reliable_f128) due to some bugginess across the backends (and across LLVM versions)

@tgross35
Copy link
Contributor

More specific list of that:

cfg.has_reliable_f128 = match (target_arch, target_os) {
// Unsupported https://github.com/llvm/llvm-project/issues/121122
(Arch::AmdGpu, _) => false,
// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
(Arch::Arm64EC, _) => false,
// Selection bug <https://github.com/llvm/llvm-project/issues/95471>. This issue is closed
// but basic math still does not work.
(Arch::Nvptx64, _) => false,
// ABI bugs <https://github.com/rust-lang/rust/issues/125109> et al. (full
// list at <https://github.com/rust-lang/rust/issues/116909>)
(Arch::PowerPC | Arch::PowerPC64, _) => false,
// ABI unsupported <https://github.com/llvm/llvm-project/issues/41838>
(Arch::Sparc, _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
(Arch::X86_64, Os::Windows) if *target_env == Env::Gnu && *target_abi != Abi::Llvm => false,
// There are no known problems on other platforms, so the only requirement is that symbols
// are available. `compiler-builtins` provides all symbols required for core `f128`
// support, so this should work for everything else.
_ => true,
};

@cuviper
Copy link
Member Author

cuviper commented Mar 19, 2026

Hmm, conditional implementation would be straightforward, but conditional testing would be quite annoying. I'll wait for feedback whether we actually want this change before I attempt that.

@clarfonthey
Copy link
Contributor

Since a software float implementation would effectively be identical to the other PR, it does feel like this might be a better long-term change, but perhaps in the short-term the other PR is better?

As in, leave a FIXME: maybe use f128 when more reliable comment but still go with that method.

@cuviper
Copy link
Member Author

cuviper commented Mar 19, 2026

@tgross35 is it possible to use explicit software mul/div from e.g. compiler-builtins on the non-reliable targets?

@tgross35
Copy link
Contributor

tgross35 commented Mar 19, 2026

We need LLVM to not crash when it sees a f128 so c-b gates the implementations behind #[cfg(target_has_reliable_f128)]. In theory we could expose the same algorithms using i128 in the signature reasonably easy, but I don't know that it's worth the effort for something that should be short term.

Could we stomach some platform differences for now and use f128 where it is well-supported?

if nanos < 0.0 {
panic!("{}", TryFromFloatSecsError { kind: TryFromFloatSecsErrorKind::Negative });
} else if nanos <= const { Self::MAX.as_nanos() as f128 } {
Self::from_nanos_u128(nanos.round_ties_even() as u128)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is subject to error due to double rounding, as the product of f128's has possibly already be rounded. Test case:

assert_eq!(Duration::MAX.mul_f64(0.5_f64.next_up()).as_nanos(), 9223372036854777855999999999); 

Validation:
https://www.wolframalpha.com/input?i=%2810%5E9*2%5E64-1%29*%282%5E52%2B1%29%2F2%5E53

That truly is an edge case, but it's part of the reason I mentioned using the subnormal range to get the correct integer rounding already at the multiplication. (It might also perform better, since it can replace the integer-float conversions with bitcasts.) Here's how that might look:
https://play.rust-lang.org/?version=nightly&mode=release&edition=2024&gist=78d6e7cde0d5016968e7db06e5896c9b

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks you for explaining, along with the clear test case. I've made a change similar to what you suggest, and I applied that to division too, although I didn't figure out a good test on the edge for that. If you know how to construct a similar division test, I'd be happy to add it!

Copy link
Contributor

@quaternic quaternic Mar 22, 2026

Choose a reason for hiding this comment

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

Could you try with 1 << 93 nanos divided by ((1 << 47) - 1) as f64 / (1 << 47) as f64? The exact value is 2^93 + 2^46 + 2^-1 + 2^-48 + ... and f128 can only hold the first three terms, so double rounding should incorrectly round the half down. I can't test it properly on mobile at this time.

edit: Playground worked well enough: playground including a bonus test case suitable for f32 as well

@scottmcm
Copy link
Member

To match #150933,
r? tgross35

@rustbot rustbot assigned tgross35 and unassigned scottmcm Mar 20, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2026

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

@cuviper
Copy link
Member Author

cuviper commented Mar 21, 2026

I added conditions for target_has_reliable_f128, including tests. It works for me on x86_64-unknown-linux-gnu using f128, and on x86_64-pc-windows-gnu (via Wine) using the fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loss of precision in Duration::mul_f32 (and mul_f64)

6 participants