| Message ID | 20201109102147.31747-1-thatlemon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/translate: Fix unordered f64/f128 comparisons | expand |
On 11/9/20 2:21 AM, LemonBoy wrote: > According to the PowerISA v3.1 reference, Table 68 "Actions for xscmpudp > - Part 1: Compare Unordered", whenever one of the two operands is a NaN > the SO bit is set while the other three bits are cleared. > > Apply the same change to xscmpuqp. > > The respective ordered counterparts are unaffected. > > Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> > --- > target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c > index 9b8c8b70b6..b07ff66375 100644 > --- a/target/ppc/fpu_helper.c > +++ b/target/ppc/fpu_helper.c > @@ -2479,13 +2479,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ > if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) || \ > float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) { \ > vxsnan_flag = true; \ > - cc = CRF_SO; \ > if (fpscr_ve == 0 && ordered) { \ > vxvc_flag = true; \ > } \ > } else if (float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) || \ > float64_is_quiet_nan(xb->VsrD(0), &env->fp_status)) { \ > - cc = CRF_SO; \ > if (ordered) { \ > vxvc_flag = true; \ > } \ > @@ -2497,12 +2495,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ > float_invalid_op_vxvc(env, 0, GETPC()); \ > } \ > \ > - if (float64_lt(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \ > + switch (float64_compare(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {\ > + case float_relation_less: \ > cc |= CRF_LT; \ > - } else if (!float64_le(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \ > - cc |= CRF_GT; \ > - } else { \ > + break; \ > + case float_relation_equal: \ > cc |= CRF_EQ; \ > + break; \ > + case float_relation_greater: \ > + cc |= CRF_GT; \ > + break; \ > + case float_relation_unordered: \ > + cc |= CRF_SO; \ > + break; \ > } This needs some more cleanup. There's no point in checking for nans first; wait until you get to float_relation_unordered. These macros should be made into straight functions. r~
On 10/11/20 04:24, Richard Henderson wrote: > On 11/9/20 2:21 AM, LemonBoy wrote: >> According to the PowerISA v3.1 reference, Table 68 "Actions for xscmpudp >> - Part 1: Compare Unordered", whenever one of the two operands is a NaN >> the SO bit is set while the other three bits are cleared. >> >> Apply the same change to xscmpuqp. >> >> The respective ordered counterparts are unaffected. >> >> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> >> --- >> target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++---------- >> 1 file changed, 22 insertions(+), 10 deletions(-) >> >> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c >> index 9b8c8b70b6..b07ff66375 100644 >> --- a/target/ppc/fpu_helper.c >> +++ b/target/ppc/fpu_helper.c >> @@ -2479,13 +2479,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ >> if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) || \ >> float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) { \ >> vxsnan_flag = true; \ >> - cc = CRF_SO; \ >> if (fpscr_ve == 0 && ordered) { \ >> vxvc_flag = true; \ >> } \ >> } else if (float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) || \ >> float64_is_quiet_nan(xb->VsrD(0), &env->fp_status)) { \ >> - cc = CRF_SO; \ >> if (ordered) { \ >> vxvc_flag = true; \ >> } \ >> @@ -2497,12 +2495,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ >> float_invalid_op_vxvc(env, 0, GETPC()); \ >> } \ >> \ >> - if (float64_lt(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \ >> + switch (float64_compare(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {\ >> + case float_relation_less: \ >> cc |= CRF_LT; \ >> - } else if (!float64_le(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \ >> - cc |= CRF_GT; \ >> - } else { \ >> + break; \ >> + case float_relation_equal: \ >> cc |= CRF_EQ; \ >> + break; \ >> + case float_relation_greater: \ >> + cc |= CRF_GT; \ >> + break; \ >> + case float_relation_unordered: \ >> + cc |= CRF_SO; \ >> + break; \ >> } > > This needs some more cleanup. There's no point in checking for nans first; > wait until you get to float_relation_unordered. > > These macros should be made into straight functions. > Will do, I tried to keep the amount of changed lines as small as possible but I'm happy to nuke that macro: aligning the trailing slashes took more time than finding and fixing the bug :) > > r~ >
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c index 9b8c8b70b6..b07ff66375 100644 --- a/target/ppc/fpu_helper.c +++ b/target/ppc/fpu_helper.c @@ -2479,13 +2479,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) || \ float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) { \ vxsnan_flag = true; \ - cc = CRF_SO; \ if (fpscr_ve == 0 && ordered) { \ vxvc_flag = true; \ } \ } else if (float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) || \ float64_is_quiet_nan(xb->VsrD(0), &env->fp_status)) { \ - cc = CRF_SO; \ if (ordered) { \ vxvc_flag = true; \ } \ @@ -2497,12 +2495,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ float_invalid_op_vxvc(env, 0, GETPC()); \ } \ \ - if (float64_lt(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \ + switch (float64_compare(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {\ + case float_relation_less: \ cc |= CRF_LT; \ - } else if (!float64_le(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \ - cc |= CRF_GT; \ - } else { \ + break; \ + case float_relation_equal: \ cc |= CRF_EQ; \ + break; \ + case float_relation_greater: \ + cc |= CRF_GT; \ + break; \ + case float_relation_unordered: \ + cc |= CRF_SO; \ + break; \ } \ \ env->fpscr &= ~FP_FPCC; \ @@ -2545,12 +2550,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode, \ float_invalid_op_vxvc(env, 0, GETPC()); \ } \ \ - if (float128_lt(xa->f128, xb->f128, &env->fp_status)) { \ + switch (float128_compare(xa->f128, xb->f128, &env->fp_status)) { \ + case float_relation_less: \ cc |= CRF_LT; \ - } else if (!float128_le(xa->f128, xb->f128, &env->fp_status)) { \ - cc |= CRF_GT; \ - } else { \ + break; \ + case float_relation_equal: \ cc |= CRF_EQ; \ + break; \ + case float_relation_greater: \ + cc |= CRF_GT; \ + break; \ + case float_relation_unordered: \ + cc |= CRF_SO; \ + break; \ } \ \ env->fpscr &= ~FP_FPCC; \
According to the PowerISA v3.1 reference, Table 68 "Actions for xscmpudp - Part 1: Compare Unordered", whenever one of the two operands is a NaN the SO bit is set while the other three bits are cleared. Apply the same change to xscmpuqp. The respective ordered counterparts are unaffected. Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> --- target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)