Re: [PATCH v8] target/ppc: Fix bugs in VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros

[Richard Henderson]

On 11/11/23 09:36, Cédric Le Goater wrote:
> Adding Richard,
>
> Thanks,
>
> C.
>
>
> On 11/10/23 19:41, John Platts wrote:
> > The patch below fixes a bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2
> > macros in target/ppc/fpu_helper.c where a non-NaN floating point value from the
> > source vector is incorrectly converted to 0, 0x80000000, or 0x8000000000000000
> > instead of the expected value if a preceding source floating point value from
> > the same source vector was a NaN.
> >
> > The bug in the VSX_CVT_FP_TO_INT and VSX_CVT_FP_TO_INT2 macros in
> > target/ppc/fpu_helper.c was introduced with commit c3f24257e3c0.
> >
> > This patch also adds a new vsx_f2i_nan test in tests/tcg/ppc64 that checks that
> > the VSX xvcvspsxws, xvcvspuxws, xvcvspsxds, xvcvspuxds, xvcvdpsxws, xvcvdpuxws,
> > xvcvdpsxds, and xvcvdpuxds instructions correctly convert non-NaN floating point
> > values to integer values if the source vector contains NaN floating point 
> > values.
> >
> > Fixes: c3f24257e3c0 ("target/ppc: Clear fpstatus flags on helpers missing it")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1941
> > Signed-off-by: John Platts <john_platts@hotmail.com>
> > ---
> >   target/ppc/fpu_helper.c         |  12 +-
> >   tests/tcg/ppc64/Makefile.target |   5 +
> >   tests/tcg/ppc64/vsx_f2i_nan.c   | 300 ++++++++++++++++++++++++++++++++
> >   3 files changed, 313 insertions(+), 4 deletions(-)
> >   create mode 100644 tests/tcg/ppc64/vsx_f2i_nan.c
> >
> > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> > index 03150a0f10..4b3dcad5d1 100644
> > --- a/target/ppc/fpu_helper.c
> > +++ b/target/ppc/fpu_helper.c
> > @@ -2880,20 +2880,22 @@ uint64_t helper_XSCVSPDPN(uint64_t xb)
> >   #define VSX_CVT_FP_TO_INT(op, nels, stp, ttp, sfld, tfld, sfi, rnan)         \
> >   void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb)             \
> >   {                                                                            \
> > +    int all_flags = 0;                                                       \
> >       ppc_vsr_t t = { };                                                       \
> >       int i, flags;                                                            \
> >                                                                                \
> > -    helper_reset_fpstatus(env);                                              \
> > -                                                                             \
> >       for (i = 0; i < nels; i++) {                                             \
> > +        helper_reset_fpstatus(env);                                          \
> >           t.tfld = stp##_to_##ttp##_round_to_zero(xb->sfld, &env->fp_status);  \
> >           flags = env->fp_status.float_exception_flags;                        \
> > +        all_flags |= flags;                                                  \
> >           if (unlikely(flags & float_flag_invalid)) {                          \
> >               t.tfld = float_invalid_cvt(env, flags, t.tfld, rnan, 0, GETPC());\
> >           }                                                                    \
> >       }                                                                        \
> >                                                                                \
> >       *xt = t;                                                                 \
> > +    env->fp_status.float_exception_flags = all_flags;                        \
> >       do_float_check_status(env, sfi, GETPC());    


Looks reasonable.  Each lane wants to check for snan, and it needs a clean set of flags to 
do that.  Merge all lane flags to pass to do_float_check_status.  So,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~