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

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

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
>
>