-
Notifications
You must be signed in to change notification settings - Fork 212
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
FPU fflags no being asserted correctly #785
Comments
Additional bugs found are 2 to 7. The cause is described below, will add a comment per bug with the recreation steps. Bug #2: Bug #3: Bug #4: Bug #5: Bug #6: Bug #7: Reference source: Some reasonable description of further "fun with float flags" stuff. IEEE 754 float reference for dealing with the sign bit and 0.0s |
Bug #2: details
Code fix in the normalizer S_ROUND state is to set in-exact if we end up shifting out a bit
|
Bug #3:
Same fix as Bug #2. |
Bug #4:
Fix applied to normalizer S_CHECK state where we miss setting inexact when the result is an underflow or overflow.
|
Bug #5:
Fix the same as Bug #4. |
Bug #6:
The fix is to modify the exception handling in the add/sub block. This is a partial fix, as I note in the comments, there might be further exceptions to the exceptions that need to be taken into account.
Addendum: Code fix updated to correctly deal with infinities based on FPU operation:
|
Bug #7.
This one has everything to do with sign handling of the result is exactly 0.0. The notes on 0.0 and signs provided above state that for fadd if the signs of rs1 and rs2 are different and the result is 0.0 it must be a +0.0 and not -0.0. Same goes for fsub, except here its for cases where rs1 and rs2 are the same sign. Off-course there is a rounding mode exception as well.
|
With the above fixes we pass the riscof-arch fadd_b1 sub-test for all cases. With some sub-normal caveats. We do pass against our Imperas DV model that has modified sub-normal handling to be equivalent to the neorv32 core, even if the inputs are sub-normal. |
With the above fixed we pass 11 out of 59 F/Zfinx riscof-arch tests. More are "passing" but the F/Zfinx riscof-arch test include sub-normals and the Sail model doesn't replace sub-normals with 0.0 as we do. |
Created PR 788 to apply the above fixes. |
Wow, this is really amazing! Thank you so much for all your feedback! I think I'll need some time to look through everything 😅 |
I found this article discussing GCC's |
Interesting, maybe. We've been experimenting with -ffast-math to help optimize down the size of the code base. Did hit a few snags with exception handling, as in the softfloat library went of the rails when a div(0) error occurred. |
I'm on the latter team, but that's just me. 😅 |
The issues identified in this issue have been patched. |
Describe the bug
This is a known issue that I finally have gotten around to. The debugging is far from complete, so would suggest I add onto this this issue with examples and fixes before putting in a PR. Alternative I can toss a PR for each, but might be a lot of back-n-forth.
bug #1: When an f.add is being issued and one of the operands is +/- 0.0 and the other operands exponent is large enough to trigger the exception path the FPU will erroneously set the NX flag. This bug was caused by an earlier bug fix from me that prevent the addsub from running forever for large exponent differences.
To Reproduce
This is the instruction test from our converted F -> Zfinx riscof-arch test.
Note gcc's asm out doesn't reproduce zfinx disassembly correctly, so the above Imperas DV output for the real assembly command.
Note when you see "sky_" in front of a riscof-arch test function name is calling an updated test function that replaces F# registers with X# registers in a manner that is compliant with riscof-arch.
Expected behavior
For bug #1 the core is not supposed to signal NX in the fflags.
The fix is to extend the addsub large exponent difference exception path with a sreg_man = 0 exception detector
Screenshots
NA
Environment:
Hardware:
Additional context
I suggest I keep extending this thread with additional bugs detected/fixes until I get a more consistent pass of our set of rv32i_m_Zfinx_XXX tests.
The text was updated successfully, but these errors were encountered: