Re: [PATCH v4 2/3] target/riscv: Use float64_to_int64_modulo for fcvtmod.w.d

[Christoph Müllner]

On Fri, Jun 30, 2023 at 11:22 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/30/23 11:13, Christoph Muellner wrote:
> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >
> > For the most part we can use the new generic routine,
> > though exceptions need some post-processing.
> >
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >   target/riscv/fpu_helper.c | 78 ++++++++++++++-------------------------
> >   1 file changed, 27 insertions(+), 51 deletions(-)
> >
> > diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> > index 289b3bbea5..0f897cf072 100644
> > --- a/target/riscv/fpu_helper.c
> > +++ b/target/riscv/fpu_helper.c
> > @@ -482,70 +482,46 @@ target_ulong helper_fcvt_w_d(CPURISCVState *env, 
> > uint64_t frs1)
> >       return float64_to_int32(frs1, &env->fp_status);
> >   }
> >
> > +/* T floating (double) */
> > +static inline float64 t_to_float64(uint64_t a)
> > +{
> > +    /* Memory format is the same as float64 */
> > +    CPU_DoubleU r;
> > +    r.ll = a;
> > +    return r.d;
> > +}
>
> You don't need this.  Nor does Alpha anymore, come to that.
> float64 is uint64_t now, always.

Ok.

>
> > +    int64_t ret;
> > +    int32_t ret32;
> > +    uint32_t e_old, e_new;
> > +    float64 fvalue;
> > +
> > +    e_old = get_float_exception_flags(status);
> > +    set_float_exception_flags(0, status);
> > +    fvalue = t_to_float64(value);
> > +    ret = float64_to_int32_modulo(fvalue, float_round_to_zero, status);
> > +    e_new = get_float_exception_flags(status);
> > +
> > +    /* Map the flags to the specified ones. */
> > +    if (e_new & float_flag_inexact) {
> > +        e_new = float_flag_inexact;
> > +    } else if (e_new) {
> > +        e_new = float_flag_invalid;
> >       }
>
> Why?  Generic code will not set both inexact and invalid.
> So this is a nop.
>
> Removing that, all of your fp flags handling can go away.

I added that because float64_to_int32_modulo() might also set
float_flag_invalid_cvti. I just realized that it is not needed,
because riscv_cpu_get_fflags() takes care to not expose
flags that are not defined for RISC-V.

>
>
> >       /* Truncate to 32-bits. */
> > -    int32_t ret32 = (int32_t)ret;
> > +    ret32 = (int32_t)ret;
> >
> >       /* If the truncation drops bits then raise NV. */
> >       if ((uint64_t)ret32 != ret)
>
> This will never fail, because you used float64_to_int32_modulo, which already 
> returns int32_t.
>
> But we have already raised invalid for overflow, so this can go away as well.

Understood.

>
> Finally, this patch must be merged with the previous, which introduced this 
> function.

Ok.

Thanks!

>
>
> r~