| Message ID | 20220905123746.54659-4-victor.colombo@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: Move fsqrt[s] to decodetree | expand |
On 9/5/22 13:37, Víctor Colombo wrote: > These two helpers are almost identical, differing only by the softfloat > operation it calls. Merge them into one using a macro. > Also, take this opportunity to capitalize the helper name as we moved > the instruction to decodetree in a previous patch. > > Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br> > --- > target/ppc/fpu_helper.c | 35 +++++++++++------------------- > target/ppc/helper.h | 4 ++-- > target/ppc/translate/fp-impl.c.inc | 4 ++-- > 3 files changed, 17 insertions(+), 26 deletions(-) > > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c > index 0f045b70f8..32995179b5 100644 > --- a/target/ppc/fpu_helper.c > +++ b/target/ppc/fpu_helper.c > @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState *env, int flags, > } > } > > -/* fsqrt - fsqrt. */ > -float64 helper_fsqrt(CPUPPCState *env, float64 arg) > -{ > - float64 ret = float64_sqrt(arg, &env->fp_status); > - int flags = get_float_exception_flags(&env->fp_status); > - > - if (unlikely(flags & float_flag_invalid)) { > - float_invalid_op_sqrt(env, flags, 1, GETPC()); > - } > - > - return ret; > +#define FPU_FSQRT(name, op) \ > +float64 helper_##name(CPUPPCState *env, float64 arg) \ > +{ \ > + float64 ret = op(arg, &env->fp_status); \ > + int flags = get_float_exception_flags(&env->fp_status); \ > + \ > + if (unlikely(flags & float_flag_invalid)) { \ > + float_invalid_op_sqrt(env, flags, 1, GETPC()); \ > + } \ Existing bug, but this is missing to clear fp status to start. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ > + \ > + return ret; \ > } > > -/* fsqrts - fsqrts. */ > -float64 helper_fsqrts(CPUPPCState *env, float64 arg) > -{ > - float64 ret = float64r32_sqrt(arg, &env->fp_status); > - int flags = get_float_exception_flags(&env->fp_status); > - > - if (unlikely(flags & float_flag_invalid)) { > - float_invalid_op_sqrt(env, flags, 1, GETPC()); > - } > - return ret; > -} > +FPU_FSQRT(FSQRT, float64_sqrt) > +FPU_FSQRT(FSQRTS, float64r32_sqrt) > > /* fre - fre. */ > float64 helper_fre(CPUPPCState *env, float64 arg) > diff --git a/target/ppc/helper.h b/target/ppc/helper.h > index 159b352f6e..68610896b8 100644 > --- a/target/ppc/helper.h > +++ b/target/ppc/helper.h > @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64) > DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64) > DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64) > DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64) > -DEF_HELPER_2(fsqrt, f64, env, f64) > -DEF_HELPER_2(fsqrts, f64, env, f64) > +DEF_HELPER_2(FSQRT, f64, env, f64) > +DEF_HELPER_2(FSQRTS, f64, env, f64) > DEF_HELPER_2(fre, i64, env, i64) > DEF_HELPER_2(fres, i64, env, i64) > DEF_HELPER_2(frsqrte, i64, env, i64) > diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc > index 7a90c0e350..8d5cf0f982 100644 > --- a/target/ppc/translate/fp-impl.c.inc > +++ b/target/ppc/translate/fp-impl.c.inc > @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a, > return true; > } > > -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt); > -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts); > +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT); > +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS); > > /*** Floating-Point multiply-and-add ***/ > /* fmadd - fmadds */
On 05/09/2022 12:56, Richard Henderson wrote: > On 9/5/22 13:37, Víctor Colombo wrote: >> These two helpers are almost identical, differing only by the softfloat >> operation it calls. Merge them into one using a macro. >> Also, take this opportunity to capitalize the helper name as we moved >> the instruction to decodetree in a previous patch. >> >> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br> >> --- >> target/ppc/fpu_helper.c | 35 +++++++++++------------------- >> target/ppc/helper.h | 4 ++-- >> target/ppc/translate/fp-impl.c.inc | 4 ++-- >> 3 files changed, 17 insertions(+), 26 deletions(-) >> >> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c >> index 0f045b70f8..32995179b5 100644 >> --- a/target/ppc/fpu_helper.c >> +++ b/target/ppc/fpu_helper.c >> @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState >> *env, int flags, >> } >> } >> >> -/* fsqrt - fsqrt. */ >> -float64 helper_fsqrt(CPUPPCState *env, float64 arg) >> -{ >> - float64 ret = float64_sqrt(arg, &env->fp_status); >> - int flags = get_float_exception_flags(&env->fp_status); >> - >> - if (unlikely(flags & float_flag_invalid)) { >> - float_invalid_op_sqrt(env, flags, 1, GETPC()); >> - } >> - >> - return ret; >> +#define FPU_FSQRT(name, >> op) \ >> +float64 helper_##name(CPUPPCState *env, float64 >> arg) \ >> +{ >> \ >> + float64 ret = op(arg, >> &env->fp_status); \ >> + int flags = >> get_float_exception_flags(&env->fp_status); \ >> + >> \ >> + if (unlikely(flags & float_flag_invalid)) >> { \ >> + float_invalid_op_sqrt(env, flags, 1, >> GETPC()); \ >> + >> } >> \ > > Existing bug, but this is missing to clear fp status to start. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > r~ > Hello Richard, thanks for your review! gen_reset_fpstatus() is called by the inline implementation in do_helper_fsqrt() before calling the helper (patch 1). It's probably better to move the call to inside the helper. >> + >> \ >> + return >> ret; \ >> } >> >> -/* fsqrts - fsqrts. */ >> -float64 helper_fsqrts(CPUPPCState *env, float64 arg) >> -{ >> - float64 ret = float64r32_sqrt(arg, &env->fp_status); >> - int flags = get_float_exception_flags(&env->fp_status); >> - >> - if (unlikely(flags & float_flag_invalid)) { >> - float_invalid_op_sqrt(env, flags, 1, GETPC()); >> - } >> - return ret; >> -} >> +FPU_FSQRT(FSQRT, float64_sqrt) >> +FPU_FSQRT(FSQRTS, float64r32_sqrt) >> >> /* fre - fre. */ >> float64 helper_fre(CPUPPCState *env, float64 arg) >> diff --git a/target/ppc/helper.h b/target/ppc/helper.h >> index 159b352f6e..68610896b8 100644 >> --- a/target/ppc/helper.h >> +++ b/target/ppc/helper.h >> @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64) >> DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64) >> DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64) >> DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64) >> -DEF_HELPER_2(fsqrt, f64, env, f64) >> -DEF_HELPER_2(fsqrts, f64, env, f64) >> +DEF_HELPER_2(FSQRT, f64, env, f64) >> +DEF_HELPER_2(FSQRTS, f64, env, f64) >> DEF_HELPER_2(fre, i64, env, i64) >> DEF_HELPER_2(fres, i64, env, i64) >> DEF_HELPER_2(frsqrte, i64, env, i64) >> diff --git a/target/ppc/translate/fp-impl.c.inc >> b/target/ppc/translate/fp-impl.c.inc >> index 7a90c0e350..8d5cf0f982 100644 >> --- a/target/ppc/translate/fp-impl.c.inc >> +++ b/target/ppc/translate/fp-impl.c.inc >> @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, >> arg_A_tb *a, >> return true; >> } >> >> -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt); >> -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts); >> +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT); >> +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS); >> >> /*** Floating-Point >> multiply-and-add ***/ >> /* fmadd - fmadds */ >
On 9/5/22 17:19, Víctor Colombo wrote: >> Existing bug, but this is missing to clear fp status to start. >> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> >> r~ >> > > Hello Richard, thanks for your review! > gen_reset_fpstatus() is called by the inline implementation in > do_helper_fsqrt() before calling the helper (patch 1). Oops, ok. > It's probably better to move the call to inside the helper. I did write about a scheme by which all of these calls should go away. I guess it has been a while... r~
On 05/09/2022 13:21, Richard Henderson wrote: > On 9/5/22 17:19, Víctor Colombo wrote: >>> Existing bug, but this is missing to clear fp status to start. >>> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> >>> r~ >>> >> >> Hello Richard, thanks for your review! >> gen_reset_fpstatus() is called by the inline implementation in >> do_helper_fsqrt() before calling the helper (patch 1). > > Oops, ok. > > >> It's probably better to move the call to inside the helper. > > I did write about a scheme by which all of these calls should go away. > I guess it has > been a while... > > > r~ I have a message bookmarked here ([1]), but I don't know if there is a previous one with a more in depth scheme. Anyway, I was also analyzing recently the idea of removing all these reset_fpstatus() calls from instructions helpers. I think this would require to actually call it from the end of the (previous) instructions instead of the beginning? Like adding the call to do_float_check_status() and float_invalid_op_*() as a focal point to 'hide' the calls to reset_fpstatus(). However there are also insns helpers that don't call these auxiliary functions, which I think would cause the refactor to not be worthy overall. Did you have another idea that could be simpler? [1] https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg00064.html
On 9/5/22 17:31, Víctor Colombo wrote: > I have a message bookmarked here ([1]), but I don't know if there is a > previous one with a more in depth scheme. There may have been a previous, but that's the one I was thinking of. > Anyway, I was also analyzing recently the idea of removing all these > reset_fpstatus() calls from instructions helpers. I think this would > require to actually call it from the end of the (previous) instructions instead of the > beginning? Like adding the call to > do_float_check_status() and float_invalid_op_*() as a focal point to > 'hide' the calls to reset_fpstatus(). Well, there would of course be no separate call, but do_float_check_status would: int status = get_float_exception_flags(&env->fp_status); set_float_exception_flags(0, &env->fp_status); straight away. No extra call overhead, and the steady-state of softfp exception flags outside of an in-progress fp operation is 0. > However there are also insns > helpers that don't call these auxiliary functions, which I think would > cause the refactor to not be worthy overall. Anything that can raise a softfp exception and doesn't do something with it, either immediately within the same helper, or just afterward with helper_float_check_status, is buggy. With those fixed, helper_reset_fpstatus may be removed entirely. r~
On 05/09/2022 14:20, Richard Henderson wrote: > Well, there would of course be no separate call, but I didn't understand what you meant here with 'no separate call'... > do_float_check_status would: > > int status = get_float_exception_flags(&env->fp_status); > > set_float_exception_flags(0, &env->fp_status); > > straight away. No extra call overhead, and the steady-state of softfp > exception flags > outside of an in-progress fp operation is 0. > Right, makes sense. And what about when an invalid operation occurs, with the corresponding exception enabled bit set? float_invalid_op_* would stop the execution and do_float_check_status would not be called, right? So it would require to call set_float_exception_flags there too, correct? If that's all that's necessary, I might be able to take a look at it and come with a possible patch. > Anything that can raise a softfp exception and doesn't do something with > it, either > immediately within the same helper, or just afterward with > helper_float_check_status, is > buggy. With those fixed, helper_reset_fpstatus may be removed entirely. > Oh, that makes sense. It's easier to implement the idea using this assumption.
On 9/6/22 15:22, Víctor Colombo wrote: > On 05/09/2022 14:20, Richard Henderson wrote: >> Well, there would of course be no separate call, but > > I didn't understand what you meant here with 'no separate call'... We generate a separate call from tcg to helper_reset_fpstatus sometimes. > Right, makes sense. And what about when an invalid operation occurs, > with the corresponding exception enabled bit set? > float_invalid_op_* would stop the execution and do_float_check_status > would not be called, right? So it would require to call > set_float_exception_flags there too, correct? Correct. r~
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c index 0f045b70f8..32995179b5 100644 --- a/target/ppc/fpu_helper.c +++ b/target/ppc/fpu_helper.c @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState *env, int flags, } } -/* fsqrt - fsqrt. */ -float64 helper_fsqrt(CPUPPCState *env, float64 arg) -{ - float64 ret = float64_sqrt(arg, &env->fp_status); - int flags = get_float_exception_flags(&env->fp_status); - - if (unlikely(flags & float_flag_invalid)) { - float_invalid_op_sqrt(env, flags, 1, GETPC()); - } - - return ret; +#define FPU_FSQRT(name, op) \ +float64 helper_##name(CPUPPCState *env, float64 arg) \ +{ \ + float64 ret = op(arg, &env->fp_status); \ + int flags = get_float_exception_flags(&env->fp_status); \ + \ + if (unlikely(flags & float_flag_invalid)) { \ + float_invalid_op_sqrt(env, flags, 1, GETPC()); \ + } \ + \ + return ret; \ } -/* fsqrts - fsqrts. */ -float64 helper_fsqrts(CPUPPCState *env, float64 arg) -{ - float64 ret = float64r32_sqrt(arg, &env->fp_status); - int flags = get_float_exception_flags(&env->fp_status); - - if (unlikely(flags & float_flag_invalid)) { - float_invalid_op_sqrt(env, flags, 1, GETPC()); - } - return ret; -} +FPU_FSQRT(FSQRT, float64_sqrt) +FPU_FSQRT(FSQRTS, float64r32_sqrt) /* fre - fre. */ float64 helper_fre(CPUPPCState *env, float64 arg) diff --git a/target/ppc/helper.h b/target/ppc/helper.h index 159b352f6e..68610896b8 100644 --- a/target/ppc/helper.h +++ b/target/ppc/helper.h @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64) DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64) DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64) DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64) -DEF_HELPER_2(fsqrt, f64, env, f64) -DEF_HELPER_2(fsqrts, f64, env, f64) +DEF_HELPER_2(FSQRT, f64, env, f64) +DEF_HELPER_2(FSQRTS, f64, env, f64) DEF_HELPER_2(fre, i64, env, i64) DEF_HELPER_2(fres, i64, env, i64) DEF_HELPER_2(frsqrte, i64, env, i64) diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc index 7a90c0e350..8d5cf0f982 100644 --- a/target/ppc/translate/fp-impl.c.inc +++ b/target/ppc/translate/fp-impl.c.inc @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a, return true; } -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt); -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts); +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT); +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS); /*** Floating-Point multiply-and-add ***/ /* fmadd - fmadds */
These two helpers are almost identical, differing only by the softfloat operation it calls. Merge them into one using a macro. Also, take this opportunity to capitalize the helper name as we moved the instruction to decodetree in a previous patch. Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br> --- target/ppc/fpu_helper.c | 35 +++++++++++------------------- target/ppc/helper.h | 4 ++-- target/ppc/translate/fp-impl.c.inc | 4 ++-- 3 files changed, 17 insertions(+), 26 deletions(-)