[Qemu-devel] [PATCH v2] ppc: Fix emulated single to double denormalized conversions

[Qemu-devel] [PATCH v2] ppc: Fix emulated single to double denormalized conversions

[Paul A. Clarke]

From: "Paul A. Clarke" <pc@us.ibm.com>

helper_todouble() was not properly converting any denormalized 32 bit
float to 64 bit double.

Fix-suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Paul A. Clarke <pc@us.ibm.com>

v2:
- Splitting patch "ppc: Three floating point fixes"; this is just one part.
- Original suggested "fix" was likely flawed.  v2 is rewritten by
  Richard Henderson (Thanks, Richard!); I reformatted the comments in a
  couple of places, compiled, and tested.
---
 target/ppc/fpu_helper.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 52bcda2..07bc905 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -73,11 +73,20 @@ uint64_t helper_todouble(uint32_t arg)
         /* Zero or Denormalized operand.  */
         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;
+            /*
+             * Denormalized operand.
+             * Shift fraction so that the msb is in the implicit bit position.
+             * Thus, shift is in the range [1:23].
+             */
+            int shift = clz32(abs_arg) - 8;
+            /*
+             * The first 3 terms compute the float64 exponent.  We then bias
+             * this result by -1 so that we can swallow the implicit bit below.
+             */
+            int exp = -126 - shift + 1023 - 1;
+
             ret |= (uint64_t)exp << 52;
-            ret |= abs_arg << (shift + 29);
+            ret += (uint64_t)abs_arg << (52 - 23 + shift);
         }
     }
     return ret;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] ppc: Fix emulated single to double denormalized conversions

[Aleksandar Markovic]

20.08.2019. 00.32, "Paul A. Clarke" <pc@us.ibm.com> је написао/ла:
>
> From: "Paul A. Clarke" <pc@us.ibm.com>
>
> helper_todouble() was not properly converting any denormalized 32 bit
> float to 64 bit double.
>
> Fix-suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
>
> v2:
> - Splitting patch "ppc: Three floating point fixes"; this is just one
part.
> - Original suggested "fix" was likely flawed.  v2 is rewritten by
>   Richard Henderson (Thanks, Richard!); I reformatted the comments in a
>   couple of places, compiled, and tested.
> ---

Paul, the fix looks great, it is also good that it is a stand-alone patch
now, and thrre is a history too, and I just want to bring to your attention
a couple of technicalities to make this patch perfect:

- our standard phrase for fix suggestion is "Suggested-by:" (without
preceeding"Fix-");

- the patch history should be preceeded by a line with three dashes ("---")
- that way it will not become a part of the permanent commit message once
the patch is applied to the main tree, and we want that, since patch
history plays its role only during review process.

Looking forward to your sending even more patches!!

Aleksandar

>  target/ppc/fpu_helper.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 52bcda2..07bc905 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -73,11 +73,20 @@ uint64_t helper_todouble(uint32_t arg)
>          /* Zero or Denormalized operand.  */
>          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;
> +            /*
> +             * Denormalized operand.
> +             * Shift fraction so that the msb is in the implicit bit
position.
> +             * Thus, shift is in the range [1:23].
> +             */
> +            int shift = clz32(abs_arg) - 8;
> +            /*
> +             * The first 3 terms compute the float64 exponent.  We then
bias
> +             * this result by -1 so that we can swallow the implicit bit
below.
> +             */
> +            int exp = -126 - shift + 1023 - 1;
> +
>              ret |= (uint64_t)exp << 52;
> -            ret |= abs_arg << (shift + 29);
> +            ret += (uint64_t)abs_arg << (52 - 23 + shift);
>          }
>      }
>      return ret;
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH v2] ppc: Fix emulated single to double denormalized conversions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

On Mon, Aug 19, 2019 at 04:42:16PM -0500, Paul A. Clarke wrote:
> From: "Paul A. Clarke" <pc@us.ibm.com>
> 
> helper_todouble() was not properly converting any denormalized 32 bit
> float to 64 bit double.
> 
> Fix-suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
> 
> v2:
> - Splitting patch "ppc: Three floating point fixes"; this is just one part.
> - Original suggested "fix" was likely flawed.  v2 is rewritten by
>   Richard Henderson (Thanks, Richard!); I reformatted the comments in a
>   couple of places, compiled, and tested.

Applied to ppc-for-4.2, thanks.

> ---
>  target/ppc/fpu_helper.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 52bcda2..07bc905 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -73,11 +73,20 @@ uint64_t helper_todouble(uint32_t arg)
>          /* Zero or Denormalized operand.  */
>          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;
> +            /*
> +             * Denormalized operand.
> +             * Shift fraction so that the msb is in the implicit bit position.
> +             * Thus, shift is in the range [1:23].
> +             */
> +            int shift = clz32(abs_arg) - 8;
> +            /*
> +             * The first 3 terms compute the float64 exponent.  We then bias
> +             * this result by -1 so that we can swallow the implicit bit below.
> +             */
> +            int exp = -126 - shift + 1023 - 1;
> +
>              ret |= (uint64_t)exp << 52;
> -            ret |= abs_arg << (shift + 29);
> +            ret += (uint64_t)abs_arg << (52 - 23 + shift);
>          }
>      }
>      return ret;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]