Add soft float addition builtins #43

mattico · 2016-08-17T23:07:34Z

TODO:

~~Move rep_clz to int module?~~
Use integer associated type for Float trait again
Module-ify Int/Float functions
Use wrapping_* ops where appropriate (everywhere?)
Finish tests
Fix from_repr(1) + from_repr(1) == INFINITY

Amanieu · 2016-08-17T23:17:42Z

You can use u32::leading_zeros, it's already available from libcore.

Amanieu · 2016-08-17T23:19:34Z

src/float.rs

+        *significand <<= shift as T;
+        T::one() - shift as T
+    }  
+}


I would suggest a simpler version of this trait:

trait Float { type Int: Sized + Copy + Clone + Sub<Output=T> + ShlAssign<T> + Shl<T, Output=T> + One; fn bits() -> u32; // Always use u32 for bit widths fn significand_bits() -> u32; fn repr(self) -> Self::Int; fn from_repr(Self::Int) -> self; fn normalize(Self::Int) -> Self::Int; }

Amanieu · 2016-08-17T23:21:09Z

I think we should move all int related stuff into an int/ subdirectory and all the float related stuff into float/. The source tree will then look like this:
src/int/mod.rs
src/int/mul.rs
src/int/udiv.rs
src/float/mod.rs
src/float/add.rs

japaric · 2016-08-17T23:21:31Z

src/float.rs

+    fn rep_clz(a: u32) -> u32 {
+        #[cfg(not(msvc))]
+        extern "C" {
+            fn __builtin_clz(a: u32) -> u32;


Where does this symbol come from? And is this even necessary? Couldn't this just use u32::leading_zeros? That method (and the u64 variant) already lower to an LLVM intrinsic: llvm.ctlz.i* in LLVM IR.

mattico · 2016-08-17T23:24:13Z

@Amanieu I don't think you can use leading_zeroes when compiling with no_builtin. Either that or I'm having some strange error that doesn't allow me to call methods on primitives.

error: no method named `leading_zeroes` found for type `u64` in the current scope

I agree with your other comments.

Amanieu · 2016-08-17T23:24:47Z

It's leading_zeros not leading_zeroes.

mattico · 2016-08-17T23:25:19Z

@Amanieu facepalm

Amanieu · 2016-08-17T23:52:35Z

src/add.rs

+            let exponent_mask =    (abs_mask ^ significand_mask) as <$ty as Float>::Int;
+            let inf_rep =          (exponent_mask) as <$ty as Float>::Int;
+            let quiet_bit =        (implicit_bit >> 1) as <$ty as Float>::Int;
+            let qnan_rep =         (exponent_mask | quiet_bit) as <$ty as Float>::Int;


You can probably get rid of all the as <$ty as Float>::Int casts here. Type inference will figure it out.

Amanieu · 2016-08-17T23:59:15Z

Can you move the transmutes into the Float trait? That way you keep all the unsafe code in one place.

fn repr(self) -> Self::Int;
fn from_repr(Self::Int) -> Self;

And generally anything that might be reused by the other soft-float implementations should go into that trait.

mattico · 2016-08-18T00:07:18Z

@Amanieu that makes a lot of sense, I'll do that.

Amanieu · 2016-08-18T01:19:33Z

src/float/add.rs

+    __adddf3(a, b)
+}
+
+#[cfg(arm)]


#[cfg(target_arch = "arm")]

Amanieu · 2016-08-18T01:24:21Z

@mattico Can you add tests for this? See the integer builtins for examples.

@japaric We'll want to make sure the tests properly cover "exotic" floating-point values like infinity and NaN.

Amanieu · 2016-08-18T01:25:01Z

For now tests with plain f32 and f64 should be enough.

mattico · 2016-08-18T01:33:21Z

@Amanieu yeah, I started writing the tests earlier, using quickcheck and some regular ones for the special cases. Doing this has identified a few issues:

Need to use wrapping_* ops to match the original C code (lots of panics)
Something's wrong with the logic somewhere (from_repr(1) + from_repr(1) == INFINITY)

I'll probably figure this out tomorrow.

japaric · 2016-08-19T01:35:29Z

We'll want to make sure the tests properly cover "exotic" floating-point values like infinity and NaN.

Yeah, we can add F32/F64 wrappers to handle this in a follow-up PR.

I started writing the tests earlier, using quickcheck and some regular ones for the special cases.

This is good enough for this PR. 👍

mattico · 2016-08-19T22:14:42Z

Rebased on master, I'll cleanup the history once this works.

japaric · 2016-08-19T23:00:36Z

src/float/add.rs

+    quickcheck! {
+        fn addsf3(a: U32, b: U32) -> bool {
+            let (a, b) = (a.0, b.0);
+            let r = super::__addsf3(<f32 as Float>::from_repr(a), <f32 as Float>::from_repr(b));


I think this can simply be f32::from_repr. Same in the f64 case.

mattico · 2016-08-20T01:14:29Z

Another thing that might cause an issue is constant folding, which might give different results than actual runtime addition.

mattico · 2016-08-20T18:18:45Z

I'm starting to think the use of Wrapping() types was a mistake. They make casts between integer types very verbose.

This last commit fixed the common qc test failure case of repr(1) + repr(1), so the tests occasionally pass, but there is still at least one case that fails sometimes: repr(1) + repr(<large number>).

japaric · 2016-08-21T02:35:41Z

src/float/add.rs

+            let (a, b) = (f32::from_repr(a.0), f32::from_repr(b.0));
+            let x = super::__addsf3(a, b);
+            let y = a + b;
+            if !(x.is_nan() && y.is_nan()) {


I personally find this easier to read as:

if x.is_nan() && y.is_nan() { true } else { x.repr() == y.repr() }

I also think this if-else block should be written as an utility function because it repeats in several places.

I that utility function should be used everywhere we need to test for float "equality" -- i.e. in all these tests.

japaric · 2016-08-21T02:46:12Z

@mattico Thanks for working on this! Looks great to me. I left some comments about nits. I'd personally like to see tests that involve +0 and -0 as well (unless they are already there and I missed them) but we can tackle those with the F32/F64 wrappers.

mattico · 2016-08-21T02:50:50Z

Thanks for the review! I'll keep these comments in mind when writing the
next one.

I do handle +-0 in the follow up PR, although I'm not sure about the method
I'm using there.

On Sat, Aug 20, 2016, 9:46 PM Jorge Aparicio [email protected]
wrote:

@mattico https://github.com/mattico Thanks for working on this! Looks
great to me. I left some comments about nits. I'd personally like to see
tests that involve +0 and -0 as well (unless they are already there and I
missed them) but we can tackle those with the F32/F64 wrappers.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#43 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0Epkx784ZFiy0TrJkIXx9dINgfBsJ6ks5qh7v1gaJpZM4Jm-l4
.

japaric · 2016-08-21T16:03:13Z

I'm going to merge this as is. We can add the utility function in a follow up PR. Thanks again @mattico!

follow up of #43

Amanieu reviewed Aug 17, 2016
View reviewed changes

japaric reviewed Aug 17, 2016
View reviewed changes

Amanieu reviewed Aug 17, 2016
View reviewed changes

Amanieu reviewed Aug 18, 2016
View reviewed changes

src/float/add.rs

__adddf3(a, b)

}

#[cfg(arm)]

Copy link

Member

Amanieu Aug 18, 2016

Choose a reason for hiding this comment

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

#[cfg(target_arch = "arm")]

japaric mentioned this pull request Aug 19, 2016

Testing: Add F32/F64 wrappers to test exotic floating point values (INF, -0, NaN) #45

Closed

mattico force-pushed the add-add_f3-builtins branch from bcdbb7e to 4eed0fe Compare August 19, 2016 22:13

japaric reviewed Aug 19, 2016
View reviewed changes

mattico force-pushed the add-add_f3-builtins branch from df90297 to 1d83529 Compare August 20, 2016 19:26

mattico changed the title ~~[WIP] Add soft float addition builtins~~ Add soft float addition builtins Aug 20, 2016

Move integer functions to separate module

6f0d50e

mattico force-pushed the add-add_f3-builtins branch from 1d83529 to 005ca06 Compare August 20, 2016 20:55

mattico mentioned this pull request Aug 20, 2016

Add float quickcheck implementations (fixes #45) #48

Merged

Implement soft float add builtins

ef16de3

mattico force-pushed the add-add_f3-builtins branch from 005ca06 to ef16de3 Compare August 20, 2016 21:06

japaric reviewed Aug 21, 2016
View reviewed changes

Use mem::swap to swap variables

da53b70

japaric merged commit ebadb12 into rust-lang:master Aug 21, 2016

japaric pushed a commit that referenced this pull request Aug 21, 2016

use utility function to compare the repr of floats

2e561b3

follow up of #43

japaric mentioned this pull request Aug 21, 2016

use utility function to compare the repr of floats #50

Merged

mattico deleted the add-add_f3-builtins branch August 21, 2016 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add soft float addition builtins #43

Add soft float addition builtins #43

mattico commented Aug 17, 2016 •

edited

Loading

Amanieu commented Aug 17, 2016

Amanieu Aug 17, 2016 •

edited

Loading

Amanieu commented Aug 17, 2016

japaric Aug 17, 2016

mattico commented Aug 17, 2016

Amanieu commented Aug 17, 2016

mattico commented Aug 17, 2016

Amanieu Aug 17, 2016

Amanieu commented Aug 17, 2016

mattico commented Aug 18, 2016

Amanieu Aug 18, 2016

Amanieu commented Aug 18, 2016

Amanieu commented Aug 18, 2016

mattico commented Aug 18, 2016 •

edited

Loading

japaric commented Aug 19, 2016

mattico commented Aug 19, 2016

japaric Aug 19, 2016

mattico commented Aug 20, 2016

mattico commented Aug 20, 2016 •

edited

Loading

japaric Aug 21, 2016

japaric Aug 21, 2016

japaric Aug 21, 2016

japaric commented Aug 21, 2016

mattico commented Aug 21, 2016

japaric commented Aug 21, 2016

Add soft float addition builtins #43

Add soft float addition builtins #43

Conversation

mattico commented Aug 17, 2016 • edited Loading

Amanieu commented Aug 17, 2016

Amanieu Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

Amanieu commented Aug 17, 2016

japaric Aug 17, 2016

Choose a reason for hiding this comment

mattico commented Aug 17, 2016

Amanieu commented Aug 17, 2016

mattico commented Aug 17, 2016

Amanieu Aug 17, 2016

Choose a reason for hiding this comment

Amanieu commented Aug 17, 2016

mattico commented Aug 18, 2016

Amanieu Aug 18, 2016

Choose a reason for hiding this comment

Amanieu commented Aug 18, 2016

Amanieu commented Aug 18, 2016

mattico commented Aug 18, 2016 • edited Loading

japaric commented Aug 19, 2016

mattico commented Aug 19, 2016

japaric Aug 19, 2016

Choose a reason for hiding this comment

mattico commented Aug 20, 2016

mattico commented Aug 20, 2016 • edited Loading

japaric Aug 21, 2016

Choose a reason for hiding this comment

japaric Aug 21, 2016

Choose a reason for hiding this comment

japaric Aug 21, 2016

Choose a reason for hiding this comment

japaric commented Aug 21, 2016

mattico commented Aug 21, 2016

japaric commented Aug 21, 2016

mattico commented Aug 17, 2016 •

edited

Loading

Amanieu Aug 17, 2016 •

edited

Loading

mattico commented Aug 18, 2016 •

edited

Loading

mattico commented Aug 20, 2016 •

edited

Loading