chipsalliance / dromajo

RISC-V RV64GC emulator designed for RTL co-simulation
Apache License 2.0
210 stars 63 forks source link

fcvt.s.w cannot handle input 0x80000000 correctly #77

Closed zephray closed 3 months ago

zephray commented 1 year ago

0x80000000 represents -2147483648 when treated as 32-bit signed integer (w), but when converting to single precision floating point number, the dromajo generates the wrong result.

It can be tested with the following code:

    li t0, 0x80000000
    fcvt.s.w f0, t0
    fmv.x.w t1, f0

The expected result is 0xCF000000 (-2147483648.0) in t1, but dromajo generates 0x80000000 (-0.0).

dpetrisko commented 6 months ago

Reporting that this issue exists for double precision as well

 li t0, 0x80000000
 fcvt.d.w f0, t0
 fmv.x.d t1, f0

Expected: c1e0000000000000 Actual: 8000000000000000

tommythorn commented 6 months ago

Thanks for the reports. I'll make a test case and investigate.

et-tommythorn commented 6 months ago

I painfully stepped through both cases and found no issue... because it only reproduces when dromajo is compiled with optimization enabled! !@#$

et-tommythorn commented 6 months ago

This seems like a compiler error. With this change, the bug goes away:

diff --git a/include/softfp_template.h b/include/softfp_template.h
index 39f2c47..79cf4ce 100644
--- a/include/softfp_template.h
+++ b/include/softfp_template.h
@@ -178,7 +178,8 @@ static F_UINT round_pack_sf(uint32_t a_sign, int a_exp, F_UINT a_mant, RoundingM
 }

 /* a_mant is considered to have at most F_SIZE - 1 bits */
-static F_UINT normalize_sf(uint32_t a_sign, int a_exp, F_UINT a_mant, RoundingModeEnum rm, uint32_t *pfflags) {
+static __attribute__ ((noinline))
+       F_UINT normalize_sf(uint32_t a_sign, int a_exp, F_UINT a_mant, RoundingModeEnum rm, uint32_t *pfflags) {
     int shift;
     shift = clz(a_mant) - (F_SIZE - 1 - IMANT_SIZE);
     assert(shift >= 0);
et-tommythorn commented 6 months ago

FWIW, it reproduces with -Ofast and -O2, but not -O

et-tommythorn commented 6 months ago

@zephray and @dpetrisko kindly provide exactly versions and build of the compiler you used to reproduce this. I can put in the workaround for now until we can get this isolated.

dpetrisko commented 6 months ago

Hi @et-tommythorn, thanks for looking into it. I compiled using Centos 7 SCL GCC 11.

gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)

I also passed -DCMAKE_BUILD_TYPE=Release if that's relevant

zephray commented 6 months ago

Hi @et-tommythorn, unfortunately as it was several months ago I am not sure which machine I reproduced the issue on, so I am not sure about the exact gcc version I used. I also passed in -DCMAKE_BUILD_TYPE=Release which enables -Ofast.

et-tommythorn commented 6 months ago

Ok, thanks @dpetrisko and @zephray. I'm going to assume you meant AMD64/x64, but it doesn't matter as it reproduces with both Arm64 and riscv64 as well as GCC 12, 13, and 14. Really funny is that the float (32-bit) case fails with Clang but double (64-bit) works. (Just to be clear, in all cases does a debug build behave as expected).

Isolating this for a regression test is ongoing, but it's not a trivial task.

EDIT: I had swapped the two clang cases around.

et-tommythorn commented 3 months ago

Credits to Owen Anderson (@resistor@mastodon.online) for suggesting building with UBSAN (-fsanitize=undefined) which pointed to the line in question. We should probably check for more UB issues.