Message ID | 20190323222412.9825-1-slyfox@gentoo.org |
---|---|
State | New |
Headers | show |
Series | powerpc: fix denorm float->double conversion | expand |
On Sat, Mar 23, 2019 at 10:24:11PM +0000, Sergei Trofimovich wrote: > The bug is initially discovered in GHC test suite. Here is minimal reproducer: > > ```c > > int main() { > volatile float f; > volatile double d; > > *(volatile uint32_t*)&f = 0xc0de; > d = f; > printf("f = %#x\n", *(volatile uint32_t*)&f); > printf("d = %#llx (expect 0x37981bc000000000)\n", > *(volatile uint64_t*)&d); > printf("d = %e\n", d); > f = d; > printf("f = %#x\n", *(volatile uint32_t*)&f); > } > ``` > > ``` > $ powerpc-unknown-linux-gnu-gcc -O2 a.c -Wall -o a \ > -fno-strict-aliasing -static && qemu-ppc ./a > f = 0xc0de > d = 0x37a00000000c0de0 (expect 0x37981bc000000000) > d = 9.183550e-41 > f = 0x10000 > ``` > > Here denormalization conversion has a few bugs: > - significand (abs_arg) has 32-bit unsigned wraparound in > ret |= abs_arg << (shift + 29); > - significand does not drop explicit leading '1' in denorm > 'float' when converting to normalized 'double' > - significand had an off-by-one shift > > CC: Richard Henderson <richard.henderson@linaro.org> > CC: David Gibson <david@gibson.dropbear.id.au> > CC: qemu-ppc@nongnu.org > CC: qemu-devel@nongnu.org > Bug: https://bugs.launchpad.net/qemu/+bug/1821444 > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> LGTM, but I don't know much about floating point. Richard, can you review this? > --- > target/ppc/fpu_helper.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c > index 2ed4f42275..1e8b014890 100644 > --- a/target/ppc/fpu_helper.c > +++ b/target/ppc/fpu_helper.c > @@ -64,13 +64,35 @@ uint64_t helper_todouble(uint32_t arg) > ret |= (uint64_t)extract32(arg, 0, 30) << 29; > } else { > /* Zero or Denormalized operand. */ > - ret = (uint64_t)extract32(arg, 31, 1) << 63; > + > + /* > + * Conversion mechanics: > + * float denorm (2^(-126) - biased): > + * [ sign (1 bit) | exp32 (8 bits) | sign32 (23 bits) ] > + * s 0 0001abc...def > + * double norm (2^(-1023) - biased): > + * [ sign (1 bit) | exp64 (11 bits) | sign64 (52 bits) ] > + * s exp abc...def 00..0 > + * Thus we are performing the following conversion steps: > + * 1. preserve the sign > + * 2. normalize denorm sign32: > + * 2a. drop explicit leading '1' as normalized numbers > + * don't contain it > + * 2b. calculate the bit-shift needed to match implicit '1' > + * 3. calculate 'exp64' as bias delta plus denorm offset > + * 4. put calculated 'sign64' into new location > + */ > + ret = (uint64_t)extract32(arg, 31, 1) << 63; /* [1.] */ > if (unlikely(abs_arg != 0)) { > /* Denormalized operand. */ > - int shift = clz32(abs_arg) - 9; > - int exp = -126 - shift + 1023; > - ret |= (uint64_t)exp << 52; > - ret |= abs_arg << (shift + 29); > + int lz = clz32(abs_arg); > + abs_arg &= ~(1 << (31 - lz)); /* [2a.] */ > + > + /* shift within sign32 includeing leading '1' */ > + int shift = lz + 1 - (32 - 23); > + int exp = -126 + 1023 - shift; /* [2b]. */ > + ret |= (uint64_t)exp << 52; /* [3.] */ > + ret |= (uint64_t)abs_arg << (52 - 23 + shift); /* [4.] */ > } > } > return ret;
On 3/23/19 12:24 PM, Sergei Trofimovich wrote: > Here denormalization conversion has a few bugs: > - significand (abs_arg) has 32-bit unsigned wraparound in > ret |= abs_arg << (shift + 29); > - significand does not drop explicit leading '1' in denorm > 'float' when converting to normalized 'double' > - significand had an off-by-one shift Correct on all points. Thanks for the test case and analysis. > + /* > + * Conversion mechanics: > + * float denorm (2^(-126) - biased): > + * [ sign (1 bit) | exp32 (8 bits) | sign32 (23 bits) ] > + * s 0 0001abc...def FWIW, the overlap between "sign" and "significand" is why I prefer the term "fraction", even though the term itself is less precise. > if (unlikely(abs_arg != 0)) { > /* Denormalized operand. */ > - int shift = clz32(abs_arg) - 9; > - int exp = -126 - shift + 1023; > - ret |= (uint64_t)exp << 52; > - ret |= abs_arg << (shift + 29); > + int lz = clz32(abs_arg); > + abs_arg &= ~(1 << (31 - lz)); /* [2a.] */ > + > + /* shift within sign32 includeing leading '1' */ > + int shift = lz + 1 - (32 - 23); > + int exp = -126 + 1023 - shift; /* [2b]. */ > + ret |= (uint64_t)exp << 52; /* [3.] */ > + ret |= (uint64_t)abs_arg << (52 - 23 + shift); /* [4.] */ I think perhaps using deposit makes things clearer, since we don't have to explicitly remove the msb in that case: E.g. @@ -67,10 +67,10 @@ uint64_t helper_todouble(uint32_t arg) ret = (uint64_t)extract32(arg, 31, 1) << 63; if (unlikely(abs_arg != 0)) { /* Denormalized operand. */ - int shift = clz32(abs_arg) - 9; - int exp = -126 - shift + 1023; - ret |= (uint64_t)exp << 52; - ret |= abs_arg << (shift + 29); + int msbm1 = 31 - clz32(abs_arg); + int exp = 1023 - 126 - (23 - msbm1); + ret = deposit64(ret, 52, 11, exp); + ret = deposit64(ret, 52 - msbm1, msbm1, abs_arg); Thoughts? r~
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c index 2ed4f42275..1e8b014890 100644 --- a/target/ppc/fpu_helper.c +++ b/target/ppc/fpu_helper.c @@ -64,13 +64,35 @@ uint64_t helper_todouble(uint32_t arg) ret |= (uint64_t)extract32(arg, 0, 30) << 29; } else { /* Zero or Denormalized operand. */ - ret = (uint64_t)extract32(arg, 31, 1) << 63; + + /* + * Conversion mechanics: + * float denorm (2^(-126) - biased): + * [ sign (1 bit) | exp32 (8 bits) | sign32 (23 bits) ] + * s 0 0001abc...def + * double norm (2^(-1023) - biased): + * [ sign (1 bit) | exp64 (11 bits) | sign64 (52 bits) ] + * s exp abc...def 00..0 + * Thus we are performing the following conversion steps: + * 1. preserve the sign + * 2. normalize denorm sign32: + * 2a. drop explicit leading '1' as normalized numbers + * don't contain it + * 2b. calculate the bit-shift needed to match implicit '1' + * 3. calculate 'exp64' as bias delta plus denorm offset + * 4. put calculated 'sign64' into new location + */ + ret = (uint64_t)extract32(arg, 31, 1) << 63; /* [1.] */ if (unlikely(abs_arg != 0)) { /* Denormalized operand. */ - int shift = clz32(abs_arg) - 9; - int exp = -126 - shift + 1023; - ret |= (uint64_t)exp << 52; - ret |= abs_arg << (shift + 29); + int lz = clz32(abs_arg); + abs_arg &= ~(1 << (31 - lz)); /* [2a.] */ + + /* shift within sign32 includeing leading '1' */ + int shift = lz + 1 - (32 - 23); + int exp = -126 + 1023 - shift; /* [2b]. */ + ret |= (uint64_t)exp << 52; /* [3.] */ + ret |= (uint64_t)abs_arg << (52 - 23 + shift); /* [4.] */ } } return ret;
The bug is initially discovered in GHC test suite. Here is minimal reproducer: ```c int main() { volatile float f; volatile double d; *(volatile uint32_t*)&f = 0xc0de; d = f; printf("f = %#x\n", *(volatile uint32_t*)&f); printf("d = %#llx (expect 0x37981bc000000000)\n", *(volatile uint64_t*)&d); printf("d = %e\n", d); f = d; printf("f = %#x\n", *(volatile uint32_t*)&f); } ``` ``` $ powerpc-unknown-linux-gnu-gcc -O2 a.c -Wall -o a \ -fno-strict-aliasing -static && qemu-ppc ./a f = 0xc0de d = 0x37a00000000c0de0 (expect 0x37981bc000000000) d = 9.183550e-41 f = 0x10000 ``` Here denormalization conversion has a few bugs: - significand (abs_arg) has 32-bit unsigned wraparound in ret |= abs_arg << (shift + 29); - significand does not drop explicit leading '1' in denorm 'float' when converting to normalized 'double' - significand had an off-by-one shift CC: Richard Henderson <richard.henderson@linaro.org> CC: David Gibson <david@gibson.dropbear.id.au> CC: qemu-ppc@nongnu.org CC: qemu-devel@nongnu.org Bug: https://bugs.launchpad.net/qemu/+bug/1821444 Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- target/ppc/fpu_helper.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)