[Qemu-devel] [PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Qemu-devel] [PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Tom Musta]

The various VSX Convert to Integer instructions should truncate the
mantissa.  This fix forces the softfloat rounding mode to "round to
zero" prior to performing the conversion.  After the conversion is
completed, the internal rounding mode is restored from the PowerPC
FPSCR bits.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
This bug was discovered when running wget, which does this:

    double maxtime;
    struct timeval tmout;
    ...
    tmout.tv_usec = 1000000 * (maxtime - (long) maxtime);

The newest PowerPC 64-bit gcc's are now using xscvdpsxds to perform the cast of 
the double to long.  A timeout of 0.95 was erroneously rounding up to 1 and
hence computing a negative timeout value.

It would be great if we could still get this into 2.0.
  
 target-ppc/fpu_helper.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index e7f3295..ccfc5cc 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -2558,10 +2558,14 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
             fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);            \
             xt.tfld = rnan;                                                  \
         } else {                                                             \
+            /* force round to zero mode (truncation) */                      \
+            set_float_rounding_mode(float_round_to_zero, &env->fp_status);   \
             xt.tfld = stp##_to_##ttp(xb.sfld, &env->fp_status);              \
             if (env->fp_status.float_exception_flags & float_flag_invalid) { \
                 fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);        \
             }                                                                \
+            /* restore rounding mode from FPSCR */                           \
+            fpscr_set_rounding_mode(env);                                    \
         }                                                                    \
     }                                                                        \
                                                                              \
-- 
1.7.1

Re: [Qemu-devel] [PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Peter Maydell]

On 23 March 2014 18:02, Tom Musta <tommusta@gmail.com> wrote:
> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index e7f3295..ccfc5cc 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -2558,10 +2558,14 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
>              fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);            \
>              xt.tfld = rnan;                                                  \
>          } else {                                                             \
> +            /* force round to zero mode (truncation) */                      \
> +            set_float_rounding_mode(float_round_to_zero, &env->fp_status);   \
>              xt.tfld = stp##_to_##ttp(xb.sfld, &env->fp_status);              \
>              if (env->fp_status.float_exception_flags & float_flag_invalid) { \
>                  fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);        \
>              }                                                                \

If we raise a CPU exception here (via helper_raise_exception_err())
we'll longjmp out of here and never restore the rounding mode. So
the restoring of the rounding mode needs to happen before we
check for exceptions here.

> +            /* restore rounding mode from FPSCR */                           \
> +            fpscr_set_rounding_mode(env);                                    \
>          }                                                                    \
>      }                                                                        \
>                                                                               \

thanks
-- PMM

[Qemu-devel] [V2 PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Tom Musta]

The various VSX Convert to Integer instructions should truncate the
mantissa.  This fix forces the softfloat rounding mode to "round to
zero" prior to performing the conversion.  After the conversion is
completed, the internal rounding mode is restored from the PowerPC
FPSCR bits.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
V2: Restored rounding mode prior to checking exceptions per Peter Maydell's
review.

This bug was discovered when running wget, which does this:

    double maxtime;
    struct timeval tmout;
    ...
    tmout.tv_usec = 1000000 * (maxtime - (long) maxtime);

The newest PowerPC 64-bit gcc's are now using xscvdpsxds to perform the cast of
the double to long.  A timeout of 0.95 was erroneously rounding up to 1 and
hence computing a negative timeout value.

It would be great if we could still get this into 2.0.

 target-ppc/fpu_helper.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index fd91239..9b3a6f7 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -2568,7 +2568,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
             fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);            \
             xt.tfld = rnan;                                                  \
         } else {                                                             \
+            /* force round to zero mode (truncation) */                      \
+            set_float_rounding_mode(float_round_to_zero, &env->fp_status);   \
             xt.tfld = stp##_to_##ttp(xb.sfld, &env->fp_status);              \
+            /* restore rounding mode from FPSCR */                           \
+            fpscr_set_rounding_mode(env);                                    \
             if (env->fp_status.float_exception_flags & float_flag_invalid) { \
                 fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);        \
             }                                                                \
-- 
1.7.1

Re: [Qemu-devel] [V2 PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Peter Maydell]

On 23 March 2014 21:52, Tom Musta <tommusta@gmail.com> wrote:
> The various VSX Convert to Integer instructions should truncate the
> mantissa.  This fix forces the softfloat rounding mode to "round to
> zero" prior to performing the conversion.  After the conversion is
> completed, the internal rounding mode is restored from the PowerPC
> FPSCR bits.
>
> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
> V2: Restored rounding mode prior to checking exceptions per Peter Maydell's
> review.
>
> This bug was discovered when running wget, which does this:
>
>     double maxtime;
>     struct timeval tmout;
>     ...
>     tmout.tv_usec = 1000000 * (maxtime - (long) maxtime);
>
> The newest PowerPC 64-bit gcc's are now using xscvdpsxds to perform the cast of
> the double to long.  A timeout of 0.95 was erroneously rounding up to 1 and
> hence computing a negative timeout value.
>
> It would be great if we could still get this into 2.0.
>
>  target-ppc/fpu_helper.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index fd91239..9b3a6f7 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -2568,7 +2568,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
>              fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);            \
>              xt.tfld = rnan;                                                  \
>          } else {                                                             \
> +            /* force round to zero mode (truncation) */                      \
> +            set_float_rounding_mode(float_round_to_zero, &env->fp_status);   \
>              xt.tfld = stp##_to_##ttp(xb.sfld, &env->fp_status);              \
> +            /* restore rounding mode from FPSCR */                           \
> +            fpscr_set_rounding_mode(env);                                    \
>              if (env->fp_status.float_exception_flags & float_flag_invalid) { \
>                  fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXCVI, 0);        \
>              }                                                                \
> --
> 1.7.1

Looking at this a little more closely, why aren't we
just using the _round_to_zero versions of the float
to int conversion softfloat functions? (This is how
we implement fctiwz vs fctiw, for instance.)

thanks
-- PMM

Re: [Qemu-devel] [V2 PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Tom Musta]

On 3/25/2014 1:06 PM, Peter Maydell wrote:

> 
> Looking at this a little more closely, why aren't we
> just using the _round_to_zero versions of the float
> to int conversion softfloat functions? (This is how
> we implement fctiwz vs fctiw, for instance.)
> 
> thanks
> -- PMM
> 

Fair question, Peter.  I will recode, retest and resubmit.  Also, I have found a rather
pervasive problem with the VSX helper code when running on LE hosts and mixing
load/store styles.  The fix touches this same code, so I will re-publish this one as part of
a series.

Re: [Qemu-devel] [V2 PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Tom Musta]

On 3/25/2014 1:06 PM, Peter Maydell wrote:
> Looking at this a little more closely, why aren't we
> just using the _round_to_zero versions of the float
> to int conversion softfloat functions? (This is how
> we implement fctiwz vs fctiw, for instance.)

Unfortunately, the softfloat list of these functions is not complete.
Specifically, float32_to_uint64_round_to_zero is missing.  I can add it
but this means a bigger patch.  Thoughts?

Re: [Qemu-devel] [V2 PATCH] target-ppc: Bug: VSX Convert to Integer Should Truncate

[Richard Henderson]

On 03/26/2014 08:05 AM, Tom Musta wrote:
> On 3/25/2014 1:06 PM, Peter Maydell wrote:
>> Looking at this a little more closely, why aren't we
>> just using the _round_to_zero versions of the float
>> to int conversion softfloat functions? (This is how
>> we implement fctiwz vs fctiw, for instance.)
> 
> Unfortunately, the softfloat list of these functions is not complete.
> Specifically, float32_to_uint64_round_to_zero is missing.  I can add it
> but this means a bigger patch.  Thoughts?
> 
Please do.


r~