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

The USE_NATIVE_FULL_ITERATIONS codepath in div64 is broken #555

Open
koute opened this issue Nov 3, 2023 · 3 comments
Open

The USE_NATIVE_FULL_ITERATIONS codepath in div64 is broken #555

koute opened this issue Nov 3, 2023 · 3 comments

Comments

@koute
Copy link
Member

koute commented Nov 3, 2023

It looks like this codepath is completely broken. If enabled like this:

diff --git a/src/float/div.rs b/src/float/div.rs
index 8c4cf55..d15c2b9 100644
--- a/src/float/div.rs
+++ b/src/float/div.rs
@@ -466,7 +466,7 @@ where
 {
     const NUMBER_OF_HALF_ITERATIONS: usize = 3;
     const NUMBER_OF_FULL_ITERATIONS: usize = 1;
-    const USE_NATIVE_FULL_ITERATIONS: bool = false;
+    const USE_NATIVE_FULL_ITERATIONS: bool = true;
 
     let one = F::Int::ONE;
     let zero = F::Int::ZERO;
@@ -922,3 +922,8 @@ intrinsics! {
         a / b
     }
 }
+
+#[test]
+fn test_div64() {
+    div64(10.0_f64, 5.0_f64);
+}

it fails with an overflow:

thread 'float::div::test_div64' panicked at 'attempt to multiply with overflow', src/float/div.rs:767:17

Looking at this code and looking at the div32 implementation I'm guessing this is how it should be fixed?

diff --git a/src/float/div.rs b/src/float/div.rs
index 8c4cf55..4ea7500 100644
--- a/src/float/div.rs
+++ b/src/float/div.rs
@@ -764,8 +764,8 @@ where
     let mut x_uq0 = if USE_NATIVE_FULL_ITERATIONS {
         for _ in 0..NUMBER_OF_FULL_ITERATIONS {
             let corr_uq1: u64 = 0.wrapping_sub(
-                (CastInto::<u64>::cast(x_uq0) * (CastInto::<u64>::cast(b_uq1))) >> F::BITS,
-            );
+                (CastInto::<u64>::cast(x_uq0) as u128 * (CastInto::<u64>::cast(b_uq1) as u128)) >> F::BITS,
+            ) as u64;
             x_uq0 = ((((CastInto::<u64>::cast(x_uq0) as u128) * (corr_uq1 as u128))
                 >> (F::BITS - 1)) as u64)
                 .cast();
@tgross35
Copy link
Contributor

tgross35 commented Aug 6, 2024

Yeah, the float division implementation is pretty fragile. The proposed fix would make f32 also use u128 math, which is better to avoid when not necessary.

Do you have a usecase where you want to tweak this? I am refactoring this as part of #622, but don'tr really plan to expose this configuration (instead it will use full iterations only if 2xBITS is <= pointer width, so it will start avoiding 64-bit math on <=32-bit platforms).

@koute
Copy link
Member Author

koute commented Aug 7, 2024

Do you have a usecase where you want to tweak this?

I don't. I just found this bug because I essentially forked this code to run in a const fn as a soft float. (So my only usecase here would be "I wish all of these functions were const fn".)

@tgross35
Copy link
Contributor

tgross35 commented Aug 7, 2024

Oh, that is an interesting idea. In theory all of these should be usable for expansion at comptime, but the use of traits probably makes it pretty hard to use directly in actual const functions. The compiler uses rustc_apfloat for its comptime math, but it is also generic.

const float math has been proposed for stabilization, so maybe you won't need this at all soon rust-lang/rust#128596.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants