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

Overhaul split integer handling and fix numerous issues #384

Merged
merged 8 commits into from
Dec 8, 2020

Conversation

AaronKutch
Copy link
Contributor

@AaronKutch AaronKutch commented Oct 13, 2020

This replaces LargInt and WideInt with the more flexible DInt and HInt. 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.

@AaronKutch
Copy link
Contributor Author

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.

@AaronKutch
Copy link
Contributor Author

I added a division test for the SPARC path. This should be ready.

@alexcrichton
Copy link
Member

@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;
Copy link
Member

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.

Copy link
Contributor Author

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 u64s 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).

Copy link
Member

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.

Copy link
Contributor Author

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

and add various methods that will be used for improved fuzzing
@AaronKutch
Copy link
Contributor Author

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). testcrate is now properly testing every numerical intrinsic except for floating point division (because it does not have subnormal handling yet) and the __powi functions (because of issues like rust-lang/rust#73920). I thought I was encountering a lot of subtle rounding mismatches on x86_64 earlier, but it turns out that I was just comparing NaNs improperly. The tests all run fine for my on my x86_64 machine. Unfortunately, i586 and i686 targets failed on the CI due to separate rounding mismatches, and I bet if the rest weren't cancelled they would also see several mismatches. Do I disable float testing on all targets except x86_64?

@AaronKutch
Copy link
Contributor Author

It looks like I just had to disable two tests to make all checks pass

@Amanieu
Copy link
Member

Amanieu commented Nov 30, 2020

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.

@AaronKutch
Copy link
Contributor Author

There is one more thing I want to do to this PR

@AaronKutch
Copy link
Contributor Author

I tried enabling some more tests by skipping subnormal numbers, but I still see things like:

thread 'float_div' panicked at '__divdf3(0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000044501477170144023): expected: 0.00000000000000011102230246251565, found: 0.00000000000000011102230246251568',

on the x86 targets, so I will enable float division testing only on x86_64.

@AaronKutch
Copy link
Contributor Author

wow, i586 cannot even handle multiplication. I have managed to completely remove WideInt and made a few more fixes for the PR. Does everything look good?

@Amanieu
Copy link
Member

Amanieu commented Dec 5, 2020

Rather than only running these tests on x86_64, could you instead just blacklist non-SSE x86 with cfg(not(all(target_arch = "x86", target_feature = "sse")))?

@AaronKutch AaronKutch force-pushed the issue-367 branch 2 times, most recently from ea6721b to 6413d9b Compare December 5, 2020 05:38
@AaronKutch
Copy link
Contributor Author

Did I find a common bug in the compiler-builtins soft conversion and in x86_64? I saw both i686 and powerpc64 fail with thread 'int_to_float' panicked at '__floatundisf(5764608897423769605): expected: 5764608600000000000, found: 5764609000000000000', testcrate\tests\conv.rs:23:5. Comparing the differences, it looks like the hardware computed found float is the correct one, being closer to the input than calculated expected.

@AaronKutch
Copy link
Contributor Author

It looks like it is failing specifically on u64 -> f32 conversions and not i64 -> f32 conversions, which means it isn't a problem with cvtsi2ss and might be an LLVM issue. I will track it down.

@Amanieu
Copy link
Member

Amanieu commented Dec 5, 2020

IIRC cvtsi2ss can only do u64 -> f32 on x86_64. I think x86 uses compiler_builtins instead.

@AaronKutch
Copy link
Contributor Author

I was confusing my self on several levels. There are several intricacies of rounding during conversion:

  • The display impl for floats is adding another layer of rounding, so the real representation needs to be used in manual comparisons
  • Most rounding in floating point uses banker's rounding, but float to int conversion just truncates which was confusing me.
  • I was mixing up failures in my mind earlier, compiler-builtins and x86_64 are correct with rounding. I made an algorithm to verify correct conversion independent of what the intrinsics or architecture calculate, and rounding towards even significand is happening correctly.
  • I was running way too few fuzzing iterations for integer to float casting. I think I knew that one of the two was more likely to have bugs but accidentally swapped the iteration numbers between float to integer casting and integer to float casting. I will just clamp the number of fuzz iterations on all tests to a minimum of 10000 to prevent this from accidentally occuring again. I was keeping iteration number low because I was concerned about the CI time on slower targets taking too long. I think what I will do is set iteration number to 10_000 for all tests on most targets, and conditionally enable a more rigorous 1_000_000 iterations on x86_64 to catch rare bugs with the algorithms themselves. My reasoning is that we only need one long run to guard against algorithmic level issues, and most other targets can have more lenient tests because problems there should only happen from freak compilation problems that are likely to completely break algorithms and only be caught with a few iterations in the worst case. I will make one CI run on all targets with 1_000_000 just to make sure I miss nothing.

@AaronKutch
Copy link
Contributor Author

The conversion test now has two parts: one that tests if the conversion is accurate (independent of whether as casts are accurate), and another that tests if the native conversion is accurate. The first part passed but the second on the same input failed on i686 with
thread 'int_to_float' panicked at '__floatundisf(5764608897423769605): expected: 1587544066, found: 1587544067', testcrate\tests\conv.rs:62:5. The found in this case is actually the true bit representation float value. expected vs. found terminology is confusing in this case, because in most tests we are using the native operations as the ground truth or "expected" value. I think I will rename expected/found to std/builtins and swap them to be less confusing.
Interestingly, this one website I was using for quick testing is bugged with the same kind of bug that i686 is affected with. It says that 5764608897423769605 converts to 5764608622545862656 with an absolute error of 274877906949, but the next higher float with integer representation 5764609172301676544 is slightly better with an error of 274877906939. I will be reporting that bug.

@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2020

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.

@AaronKutch
Copy link
Contributor Author

AaronKutch commented Dec 8, 2020

This PR is revealing all kinds of edge cases. I witnessed this configuration bug.

adds testing for almost every numerical intrinsic
@AaronKutch
Copy link
Contributor Author

I got all checks to pass with disabling as few tests as I could

@Amanieu Amanieu merged commit b8c2585 into rust-lang:master Dec 8, 2020
@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2020

Great work!

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

Successfully merging this pull request may close these issues.

#[aapcs_on_arm] is applied to two functions with no floating point
3 participants