-
Notifications
You must be signed in to change notification settings - Fork 213
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
Overhaul split integer handling and fix numerous issues #384
Conversation
I have tried to make a fuzzer for floating point numbers, but have already run into situations like "expected 0, found 0" where there are slight bit pattern disagreements between the hardware and software implementations. I might figure out a solution, but for now I would just merge this PR and I might have the time to follow up with refactors for the floating point part of the library later. |
bb57f6e
to
6241248
Compare
I added a division test for the SPARC path. This should be ready. |
@Amanieu would you be up for reviewing this? |
src/int/mod.rs
Outdated
/// Widens the integer to have double bit width and shifts the integer into the higher bits | ||
fn widen_hi(self) -> Self::D; | ||
/// Constructs a double bit width integer using lower and higher parts | ||
fn from_lo_hi(lo: Self, hi: Self) -> Self::D; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this should be a method on DInt
rather than HInt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the opposite. I think it should be consistent with the other methods in DInt
which all return Self::D
, and all the methods in HInt
return Self::H
. Consider if we added a widening_mul
method to the standard library (and I named it widen_mul
on this trait to avoid colliding with this rfc if it were ever implemented). You would widen multiply a pair of u64
s into a u128
, so it would be called like u64.widening_mul(u64) -> u128
. In a similar way, I want the signature of from_lo_hi
to be used like u128::from_lo_hi(u64, u64) -> u128
(and I use this particular signature to mirror the tuple from HInt::lo_hi
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently from_lo_hi
is called as u64::from_lo_hi(u64, u64) -> u128
which doesn't make sense. You want u128:from_lo_hi
instead of u64::from_lo_hi
which requires the method to be on the DInt
trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I don't know why I didn't see this before. It eliminates some code size in practice also
6241248
to
544983c
Compare
and add various methods that will be used for improved fuzzing
544983c
to
e964b37
Compare
I just completely rebased this PR with much better testing code (there may be a lot more macros now, but I believe they are better than copy and paste by having a smaller chance of causing false-positive bugs). |
It looks like I just had to disable two tests to make all checks pass |
I find it surprising the i686 would fail since it uses SSE instructions for floats which are IEEE compliant. It's i586 that is a bit special since it uses 80-bit float precision internally in the x87 FPU. |
There is one more thing I want to do to this PR |
I tried enabling some more tests by skipping subnormal numbers, but I still see things like:
on the x86 targets, so I will enable float division testing only on x86_64. |
wow, |
Rather than only running these tests on x86_64, could you instead just blacklist non-SSE x86 with |
ea6721b
to
6413d9b
Compare
Did I find a common bug in the |
It looks like it is failing specifically on |
IIRC cvtsi2ss can only do |
I was confusing my self on several levels. There are several intricacies of rounding during conversion:
|
The conversion test now has two parts: one that tests if the conversion is accurate (independent of whether |
I had a look at the generated code for i686 (godbolt) and it seems to be using x87 instructions with 80-bit precision which will cause rounding errors. I think it's fine to just exclude x86 entirely here. |
This PR is revealing all kinds of edge cases. I witnessed this configuration bug. |
adds testing for almost every numerical intrinsic
I got all checks to pass with disabling as few tests as I could |
Great work! |
This replaces
LargInt
andWideInt
with the more flexibleDInt
andHInt
. I also factored out common fuzzing code and added test coverage for almost every numerical intrinsic in the crate. Fixes #373, #367, and a lot of warnings that were introduced in a recent nightly. The only issue I am aware of is that the__rust
overflowing shift functions cannot be tested with overflow in debug mode (they panic from overshifting). This existed before my PR, and I'm not sure if I should fix it.