openhwgroup / cvfpu

Parametric floating-point unit with support for standard RISC-V formats and operations as well as transprecision formats.
Apache License 2.0
432 stars 115 forks source link

Errors on berkeley testfloat-3e #17

Open aignacio opened 5 years ago

aignacio commented 5 years ago

Hi pulp-team, I executed the berkeley testfloat-3e in the verilator model of ri5cy core with _FPU+DIVSQRT and here are the logs of all tests of unary and binary functions. I know that it's quite common to find errors through this set of tests on any CPU but it might be helpful to check IEEE 754 compliance and what you want/or not to be compliant. Logs attached =)

Ps.: it was used the latest master branches on fpnew/riscv core (...e1910/...813a7) Ps2.: testfloat/softfloat were compile with march:rv32imafc / mabi:ilp32f / O2

log_testfloat_3e_unary_riscv_none_embedded_ri5cy_verilator_model.txt log_testfloat_3e_binary_riscv_none_embedded_ri5cy_verilator_model.txt

stmach commented 5 years ago

Hello and thank you for using PULP and taking some time to run tests! I'll have a detailed look at the results of your test runs but here are some preliminary comments from skimming through the logs:

In any case, we'll investigate these logs to make sure any actual mismatches to the standard / specification are fixed. Thanks!

aignacio commented 5 years ago

Sure @stmach , indeed I did not know this implementation specific points that you raised about, I'll try to check it to see if it's easy to isolate without making the tesfloat so different and these logs are for taking notes about the tests itself, I'm not gonna push more work on the compliance I've know that you have being work hard on that :neckbeard:

hyf6661669 commented 5 years ago

@stmach Could you please tell me the bugs that are found in DIVSQRT unit in the latest version of fpunew? I failed to find them...

stmach commented 5 years ago

We know of the DIVSQRT unit not faithfully performing the round to nearest, tie to even rounding mode (which unfortunately is the default). This can lead to results being off by 1ulp, and the inexact flag not being properly raised in these cases as well.

smtdta commented 4 years ago

@stmach
could you clarify a doubt of mine ?

1

As per my understanding goes, the attached above soft float source code for rounding says if the result before rounding is +inf and mode is round_min , the result will be rounded to +inf-1 i.e, (+FF.000000 -1=+FE.7FFFFF) and vice-versa for round_max , where -inf (-FF.000000) will be rounded to -FE.7FFFFF if mode is round_max . and for rounding min_mag(towards zero) , both +inf and -inf will be rounded . but it wont affect round_near_even(default) .

(the log generated by @aignacio also highlights the mismatch of core results and softfloat expected results) .

having said that , the core doesn't seem to oblige this . i would be gratified if you could clarify this . sorry, for naive questions though :P :P

stmach commented 4 years ago

Hi @smtdta, sorry for the late reply to this. Yes, your observation is mostly correct: if the true result of an FP operation is larger than the largest representable FP value FLT_MAX, the result can be either FLT_MAX or inf, determined by the rounding mode:

You'll see that round_near_even and round_near_maxMag are the same in their behavior (for different reasons) in this case. For negative FLT_MAX / -inf, the only thing that changes is that round_min and round_max (and the sign of the result of course) are swapped.

Please note that this does not apply if the result before rounding is actually inf (such as for when an input was inf, or you hit a special case such as division by 0). A true infinity can never be rounded down to FLT_MAX, as it's infinitely far away from FLT_MAX.


As for the status of the FPU, as said the division & square root unit doesn't always get rounding right. We'll be updating the division & square root unit in the coming months to a new version that should be completely IEEE-compliant (and have more configurability). The other issue regarding rouding of signed zeroes in the multiplier is being investigated.

smtdta commented 4 years ago

yes , i've figured out as RISCV spec mandates ..only for infinity by overflow cases the above should fail(like div of a normal no with subnormal).

Thanks

smtdta commented 4 years ago
Screenshot 2020-05-04 at 7 24 02 PM
polkeem commented 4 years ago

Hi, any update on the status of the rounding fix? Thanks!

As for the status of the FPU, as said the division & square root unit doesn't always get rounding right. We'll be updating the division & square root unit in the coming months to a new version that should be completely IEEE-compliant (and have more configurability). The other issue regarding rouding of signed zeroes in the multiplier is being investigated.