| Message ID | 1483615579-17618-4-git-send-email-nikunj@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 05, 2017 at 04:56:08PM +0530, Nikunj A Dadhania wrote: > From: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Use float64 argument instead of unit64_t in helper_compute_fprf() > This allows code in helper_compute_fprf() to be reused later to > work with float128 argument too. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> Uh.. how can this possibly be correct, without updating the callers of helper_compute_fprf()? > --- > target-ppc/fpu_helper.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c > index 1ccd5e6..4da991a 100644 > --- a/target-ppc/fpu_helper.c > +++ b/target-ppc/fpu_helper.c > @@ -66,23 +66,21 @@ static inline int ppc_float64_get_unbiased_exp(float64 f) > return ((f >> 52) & 0x7FF) - 1023; > } > > -void helper_compute_fprf(CPUPPCState *env, uint64_t arg) > +void helper_compute_fprf(CPUPPCState *env, float64 arg) > { > - CPU_DoubleU farg; > int isneg; > int fprf; > > - farg.ll = arg; > - isneg = float64_is_neg(farg.d); > - if (unlikely(float64_is_any_nan(farg.d))) { > - if (float64_is_signaling_nan(farg.d, &env->fp_status)) { > + isneg = float64_is_neg(arg); > + if (unlikely(float64_is_any_nan(arg))) { > + if (float64_is_signaling_nan(arg, &env->fp_status)) { > /* Signaling NaN: flags are undefined */ > fprf = 0x00; > } else { > /* Quiet NaN */ > fprf = 0x11; > } > - } else if (unlikely(float64_is_infinity(farg.d))) { > + } else if (unlikely(float64_is_infinity(arg))) { > /* +/- infinity */ > if (isneg) { > fprf = 0x09; > @@ -90,7 +88,7 @@ void helper_compute_fprf(CPUPPCState *env, uint64_t arg) > fprf = 0x05; > } > } else { > - if (float64_is_zero(farg.d)) { > + if (float64_is_zero(arg)) { > /* +/- zero */ > if (isneg) { > fprf = 0x12; > @@ -98,7 +96,7 @@ void helper_compute_fprf(CPUPPCState *env, uint64_t arg) > fprf = 0x02; > } > } else { > - if (isden(farg.d)) { > + if (isden(arg)) { > /* Denormalized numbers */ > fprf = 0x10; > } else {
On Fri, Jan 06, 2017 at 09:01:17AM +1100, David Gibson wrote: > On Thu, Jan 05, 2017 at 04:56:08PM +0530, Nikunj A Dadhania wrote: > > From: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Use float64 argument instead of unit64_t in helper_compute_fprf() > > This allows code in helper_compute_fprf() to be reused later to > > work with float128 argument too. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > Uh.. how can this possibly be correct, without updating the callers of > helper_compute_fprf()? It works currently because uint64_t argument that is passed by the callers is interpreted as float64 arg (via farg.d). Let me see if there is a cleaner way of doing this. Casting the callers with right floatXX type seems to be the easiest way. The requirement here is to ensure that helper_compute_fprf_float16(CPUPPCState *env, float16 arg) helper_compute_fprf_float32(CPUPPCState *env, float32 arg) helper_compute_fprf_float64(CPUPPCState *env, float64 arg) helper_compute_fprf_float128(CPUPPCState *env, float128 arg) get autogenerated with right floatXX argument so that it can directly be used with required routines (like floatXX_is_any_nan etc) w/o needing the CPU_DoubleU or other intermediate forms. Regards, Bharata.
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Thu, Jan 05, 2017 at 04:56:08PM +0530, Nikunj A Dadhania wrote: >> From: Bharata B Rao <bharata@linux.vnet.ibm.com> >> >> Use float64 argument instead of unit64_t in helper_compute_fprf() >> This allows code in helper_compute_fprf() to be reused later to >> work with float128 argument too. >> >> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > Uh.. how can this possibly be correct, without updating the callers of > helper_compute_fprf()? Before the patch 1791 #define VSX_ADD_SUB(name, op, nels, tp, fld, sfprf, r2sp) \ 1792 void helper_##name(CPUPPCState *env, uint32_t opcode) \ 1793 { \ [SNIP] 1816 \ 1817 if (r2sp) { \ 1818 xt.fld = helper_frsp(env, xt.fld); \ 1819 } \ 1820 \ 1821 if (sfprf) { \ 1822 helper_compute_fprf(env, xt.fld); \ [SNIP] 1829 VSX_ADD_SUB(xsadddp, add, 1, float64, VsrD(0), 1, 0) 1830 VSX_ADD_SUB(xsaddsp, add, 1, float64, VsrD(0), 1, 1) 1831 VSX_ADD_SUB(xvadddp, add, 2, float64, VsrD(i), 0, 0) 1832 VSX_ADD_SUB(xvaddsp, add, 4, float32, VsrW(i), 0, 0) 1833 VSX_ADD_SUB(xssubdp, sub, 1, float64, VsrD(0), 1, 0) So we use xt.fld, which in turn will be xt.float64/xt.float32 etc. I have seen all the other path, should be fine. target/ppc/fpu_helper.c:1877: helper_compute_fprf(env, xt.fld); \ target/ppc/fpu_helper.c:1931: helper_compute_fprf(env, xt.fld); \ target/ppc/fpu_helper.c:1972: helper_compute_fprf(env, xt.fld); \ target/ppc/fpu_helper.c:2021: helper_compute_fprf(env, xt.fld); \ target/ppc/fpu_helper.c:2071: helper_compute_fprf(env, xt.fld); \ target/ppc/fpu_helper.c:2271: helper_compute_fprf(env, xt_out.fld); \ target/ppc/fpu_helper.c:2661: helper_compute_fprf(env, ttp##_to_float64(xt.tfld, \ target/ppc/fpu_helper.c:2772: helper_compute_fprf(env, xt.tfld); \ target/ppc/fpu_helper.c:2828: helper_compute_fprf(env, xt.fld); \ Except the below one, the register that we pass comes as i64 via the helper: target/ppc/helper.h:64:DEF_HELPER_2(compute_fprf, void, env, i64) Regards Nikunj
On Fri, Jan 06, 2017 at 10:27:46AM +0530, Bharata B Rao wrote: > On Fri, Jan 06, 2017 at 09:01:17AM +1100, David Gibson wrote: > > On Thu, Jan 05, 2017 at 04:56:08PM +0530, Nikunj A Dadhania wrote: > > > From: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > Use float64 argument instead of unit64_t in helper_compute_fprf() > > > This allows code in helper_compute_fprf() to be reused later to > > > work with float128 argument too. > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > > > Uh.. how can this possibly be correct, without updating the callers of > > helper_compute_fprf()? > > It works currently because uint64_t argument that is passed by the > callers is interpreted as float64 arg (via farg.d). > > Let me see if there is a cleaner way of doing this. Casting the > callers with right floatXX type seems to be the easiest way. > The requirement here is to ensure that > > helper_compute_fprf_float16(CPUPPCState *env, float16 arg) > helper_compute_fprf_float32(CPUPPCState *env, float32 arg) > helper_compute_fprf_float64(CPUPPCState *env, float64 arg) > helper_compute_fprf_float128(CPUPPCState *env, float128 arg) > > get autogenerated with right floatXX argument so that it can directly > be used with required routines (like floatXX_is_any_nan etc) w/o > needing the CPU_DoubleU or other intermediate forms. Ok. I think some of this explanation needs to go into the commit message.
diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c index 1ccd5e6..4da991a 100644 --- a/target-ppc/fpu_helper.c +++ b/target-ppc/fpu_helper.c @@ -66,23 +66,21 @@ static inline int ppc_float64_get_unbiased_exp(float64 f) return ((f >> 52) & 0x7FF) - 1023; } -void helper_compute_fprf(CPUPPCState *env, uint64_t arg) +void helper_compute_fprf(CPUPPCState *env, float64 arg) { - CPU_DoubleU farg; int isneg; int fprf; - farg.ll = arg; - isneg = float64_is_neg(farg.d); - if (unlikely(float64_is_any_nan(farg.d))) { - if (float64_is_signaling_nan(farg.d, &env->fp_status)) { + isneg = float64_is_neg(arg); + if (unlikely(float64_is_any_nan(arg))) { + if (float64_is_signaling_nan(arg, &env->fp_status)) { /* Signaling NaN: flags are undefined */ fprf = 0x00; } else { /* Quiet NaN */ fprf = 0x11; } - } else if (unlikely(float64_is_infinity(farg.d))) { + } else if (unlikely(float64_is_infinity(arg))) { /* +/- infinity */ if (isneg) { fprf = 0x09; @@ -90,7 +88,7 @@ void helper_compute_fprf(CPUPPCState *env, uint64_t arg) fprf = 0x05; } } else { - if (float64_is_zero(farg.d)) { + if (float64_is_zero(arg)) { /* +/- zero */ if (isneg) { fprf = 0x12; @@ -98,7 +96,7 @@ void helper_compute_fprf(CPUPPCState *env, uint64_t arg) fprf = 0x02; } } else { - if (isden(farg.d)) { + if (isden(arg)) { /* Denormalized numbers */ fprf = 0x10; } else {