Skip to content

Fix wrong rounding behaviour about div_euclid with fma operation. - #158154

Closed
Neutron3529 wants to merge 2 commits into
rust-lang:mainfrom
Neutron3529:main
Closed

Neutron3529 wants to merge 2 commits into
rust-lang:mainfrom
Neutron3529:main

Conversation

@Neutron3529

Copy link
Copy Markdown
Contributor

Fixes 107904. Code are written manually and PR summarized by deepseek V4.

(Since my English is not very well, the rest of PR are generated by deepseek.)

Problem

f32::div_euclid and f64::div_euclid can return an incorrect quotient when the
intermediate division a / b is rounded by the hardware to a value that
crosses an integer boundary. This happens because the current implementation
uses (a / b).trunc() as the initial guess and then adjusts based on the
remainder sign. If a / b is already wrong by one unit in the last place
(especially near multiples of 1.0), the final result may be off by 1.

Minimal example

let a = 11.0f32;
let b = 2.2f32;
// 2.2f32 is actually slightly larger than 2.2, so a / b ≈ 4.99999989...
// which is rounded to 5.0 by the default round-ties-to-even mode.
assert_eq!(a.div_euclid(b), 5.0); // wrong, mathematical quotient is 4

The true Euclidean quotient (floor division for positive b) should be 4.0.

Proposed Solution

Replace the naive (a / b).trunc() step with a guarded computation that
detects and corrects this specific rounding error.

For f32 the new implementation is:

    pub fn div_euclid(x: f32, rhs: f32) -> f32 {
        // Use floor() directly as the initial guess (Euclidean division
        // with positive divisor is the same as floor division).
        let q = (x / rhs).floor();
        // Compute b * q - a with a single rounding error.
        let diff = rhs.mul_add(q, -x);
        if diff > 0.0 { // take care of NaN: NaN > 0.0 is false, keeping the return result.
            if rhs > 0.0 {
                // q was one unit too high – move down to the next representable value.
                q.next_down().floor()
            } else {
                q.next_up().ceil()
            }
        } else {
            q
        }
    }

Things are similar to other float types.

Why this works:

  • diff = b * q - a is computed with a single rounding (via mul_add).
  • If q is truly less than or equal to a / b, then diff <= 0.0.
  • If q is one integer above the true quotient, diff > 0.0 and we adjust q by moving one floating-point unit towards zero before reapplying the rounding (.floor() or .ceil() as appropriate).

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2026
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 19, 2026
@rustbot

rustbot commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

r? @clarfonthey

rustbot has assigned @clarfonthey.
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: libs
  • libs expanded to 11 candidates
  • Random selection from Darksonn, Mark-Simulacrum, clarfonthey, jhpratt

@rust-log-analyzer

This comment has been minimized.

@tgross35

Copy link
Copy Markdown
Member

r? me

There is an alternative PR open that does more of a soft float routine. I assume this may be more performant with hardware fma, but did you look into any of the tradeoffs?

These routines should be added to libm within the compiler-builtins repo instead then later wired up to core, we have significantly better test infra there for evaluating float routines. If you need any help, let me know and I can give more details (or ping me on Zulip).

Also, please handwrite PR descriptions and commit messages. It’s completely fine if your English isn’t great - a short description from you is way more meaningful than anything an AI could ever create, especially since it’s just restating what’s literally in the diff.

@rustbot rustbot assigned tgross35 and unassigned clarfonthey Jun 19, 2026
if diff > 0.0 { // take care of NaN: NaN > 0.0 is false, keeping the return result.
if rhs > 0.0 {
// q was one unit too high – move down to the next representable value.
q.next_down().floor()

@dotacow dotacow Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: is this not effectively q - 1.0, since q is already floored and thus an integer value?

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This trying keeps q * rhs <= left always hold.
for larger floats.q-1.0 may equals to q itself, which might cause q * rhs <= left not hold.

Maybe it is worth discussion that whether we need this version of div_euclid?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, yeah the logic holds.

I went over the paper linked in #107904, and your implementation seems to fall in line nicely with its general outline, LGTM.

Maybe it is worth discussion that whether we need this version of div_euclid?

This is arguably a slower version (in contrast to the previous implementation), but it is correct. I think delegating the implementation to user level would just cause many of them to fall in the same trap.

@Neutron3529

Copy link
Copy Markdown
Contributor Author

r? me

There is an alternative PR open that does more of a soft float routine. I assume this may be more performant with hardware fma, but did you look into any of the tradeoffs?

These routines should be added to libm within the compiler-builtins repo instead then later wired up to core, we have significantly better test infra there for evaluating float routines. If you need any help, let me know and I can give more details (or ping me on Zulip).

Also, please handwrite PR descriptions and commit messages. It’s completely fine if your English isn’t great - a short description from you is way more meaningful than anything an AI could ever create, especially since it’s just restating what’s literally in the diff.

I'll try put everything in libm in case I have spare time.(maybe some workdays in the nextweek) Thank you for your patience

@tgross35

Copy link
Copy Markdown
Member

#107904 is the relevant issue here btw

@tgross35

Copy link
Copy Markdown
Member

The previous attempts were #133485 and #134145. There were a handful of concerns brought up in the first one, please make sure that this implementation (mathematically) covers them, and that there are test cases where possible. Once this moves to libm, we'll be able to do e.g. exhaustive tests on f16 against the simple round-down implementation (via rug) as well.

I believe @quaternic had looked at this in the past and may have some thoughts here.

@tgross35

Copy link
Copy Markdown
Member

Updating status,

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2026
@rustbot

rustbot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 13, 2026
@rustbot

rustbot commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Any special-casing of Miri in the standard library requires review.

cc @rust-lang/miri

@rustbot

rustbot commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

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.

@rustbot

rustbot commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Warning ⚠️

  • The following commits have merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

    You can start a rebase with the following commands:

    $ # rebase
    $ git pull --rebase https://github.com/rust-lang/rust.git main
    $ git push --force-with-lease
    

@rustbot rustbot added the has-merge-commits PR has merge commits, merge with caution. label Sep 19, 2026
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job test-tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
downloading https://static.rust-lang.org/dist/2026-08-30/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/2026-08-30/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt
[TIMING:end] format::InternalRustfmt {  } -- 12.077
Diff in /checkout/library/core/src/num/f64.rs:2149:
         let q = (x / rhs).floor();
         // Compute b * q - a with a single rounding error.
         let diff = rhs.mul_add(q, -x);
-        if diff > 0.0 { // take care of NaN: NaN > 0.0 is false, keeping the return result.
+        if diff > 0.0 {
+            // take care of NaN: NaN > 0.0 is false, keeping the return result.
             if rhs > 0.0 {
                 // q was one unit too high – move down to the next representable value.
                 q.next_down().floor()
Diff in /checkout/library/core/src/num/f128.rs:2033:
         let q = (self / rhs).floor();
         // Compute b * q - a with a single rounding error.
         let diff = rhs.mul_add(q, -self);
-        if diff > 0.0 { // take care of NaN: NaN > 0.0 is false, keeping the return result.
+        if diff > 0.0 {
+            // take care of NaN: NaN > 0.0 is false, keeping the return result.
             if rhs > 0.0 {
                 // q was one unit too high – move down to the next representable value.
                 q.next_down().floor()
Diff in /checkout/library/core/src/num/f32.rs:2171:
         let q = (x / rhs).floor();
         // Compute b * q - a with a single rounding error.
         let diff = rhs.mul_add(q, -x);
-        if diff > 0.0 { // take care of NaN: NaN > 0.0 is false, keeping the return result.
+        if diff > 0.0 {
+            // take care of NaN: NaN > 0.0 is false, keeping the return result.
             if rhs > 0.0 {
                 // q was one unit too high – move down to the next representable value.
                 q.next_down().floor()
Diff in /checkout/library/core/src/num/f16.rs:2019:
         let q = (self / rhs).floor();
         // Compute b * q - a with a single rounding error.
         let diff = rhs.mul_add(q, -self);
-        if diff > 0.0 { // take care of NaN: NaN > 0.0 is false, keeping the return result.
+        if diff > 0.0 {
+            // take care of NaN: NaN > 0.0 is false, keeping the return result.
             if rhs > 0.0 {
                 // q was one unit too high – move down to the next representable value.
                 q.next_down().floor()
fmt: checked 7277 files
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Currently active steps:
test::Tidy {  } at src/bootstrap/src/core/build_steps/test.rs:1665
Build completed unsuccessfully in 0:00:43

@Neutron3529
Neutron3529 marked this pull request as draft September 20, 2026 01:58
@Neutron3529

Copy link
Copy Markdown
Contributor Author

I have tested the code with llm, found that, there are about ~2% of f16 pairs that has inaccurate rounding results. most of them occurs in a situation that the quotient does not a valid f16 item, next_up/down force the calculated quotient to be changed, an alternative method, use q-1.0 / q+1.0 will force the calculated result not change, which might be more accurate, but cannot got the expected "ties to even" result.

I'll try it after I have a better idea..

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-merge-commits PR has merge commits, merge with caution. 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.

6 participants