| Message ID | 20230101144339.60307-3-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | target/m68k: fix FPSR quotient byte for fmod and frem | expand |
Le 01/01/2023 à 15:43, Mark Cave-Ayland a écrit : > This enables the quotient parameter to be changed from int32_t to uint32_t and > also allows the extra sign logic in make_quotient() to be removed. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > target/m68k/fpu_helper.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Le 01/01/2023 à 15:43, Mark Cave-Ayland a écrit : > This enables the quotient parameter to be changed from int32_t to uint32_t and > also allows the extra sign logic in make_quotient() to be removed. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > target/m68k/fpu_helper.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c > index 0932c464fd..ae839785fa 100644 > --- a/target/m68k/fpu_helper.c > +++ b/target/m68k/fpu_helper.c > @@ -515,15 +515,8 @@ uint32_t HELPER(fmovemd_ld_postinc)(CPUM68KState *env, uint32_t addr, > return fmovem_postinc(env, addr, mask, cpu_ld_float64_ra); > } > > -static void make_quotient(CPUM68KState *env, int32_t quotient) > +static void make_quotient(CPUM68KState *env, int sign, uint32_t quotient) > { > - int sign; > - > - sign = quotient < 0; > - if (sign) { > - quotient = -quotient; > - } > - > quotient = (sign << 7) | (quotient & 0x7f); > env->fpsr = (env->fpsr & ~FPSR_QT_MASK) | (quotient << FPSR_QT_SHIFT); > } > @@ -536,7 +529,8 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) > return; > } > > - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); > + make_quotient(env, extractFloatx80Sign(res->d), > + floatx80_to_int32(res->d, &env->fp_status)); > } > > void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) > @@ -547,7 +541,8 @@ void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) > return; > } > > - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); > + make_quotient(env, extractFloatx80Sign(res->d), > + floatx80_to_int32(res->d, &env->fp_status)); > } > > void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val) I think you need an "abs(floatx80_to_int32())" in both cases as you do in PATCH 4 Thanks, Laurent
On 1/1/23 09:26, Laurent Vivier wrote: > Le 01/01/2023 à 15:43, Mark Cave-Ayland a écrit : >> This enables the quotient parameter to be changed from int32_t to uint32_t and >> also allows the extra sign logic in make_quotient() to be removed. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> target/m68k/fpu_helper.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c >> index 0932c464fd..ae839785fa 100644 >> --- a/target/m68k/fpu_helper.c >> +++ b/target/m68k/fpu_helper.c >> @@ -515,15 +515,8 @@ uint32_t HELPER(fmovemd_ld_postinc)(CPUM68KState *env, uint32_t addr, >> return fmovem_postinc(env, addr, mask, cpu_ld_float64_ra); >> } >> -static void make_quotient(CPUM68KState *env, int32_t quotient) >> +static void make_quotient(CPUM68KState *env, int sign, uint32_t quotient) >> { >> - int sign; >> - >> - sign = quotient < 0; >> - if (sign) { >> - quotient = -quotient; >> - } >> - >> quotient = (sign << 7) | (quotient & 0x7f); >> env->fpsr = (env->fpsr & ~FPSR_QT_MASK) | (quotient << FPSR_QT_SHIFT); >> } >> @@ -536,7 +529,8 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg >> *val1) >> return; >> } >> - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); >> + make_quotient(env, extractFloatx80Sign(res->d), >> + floatx80_to_int32(res->d, &env->fp_status)); >> } >> void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) >> @@ -547,7 +541,8 @@ void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg >> *val1) >> return; >> } >> - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); >> + make_quotient(env, extractFloatx80Sign(res->d), >> + floatx80_to_int32(res->d, &env->fp_status)); >> } >> void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val) > > I think you need an "abs(floatx80_to_int32())" in both cases as you do in PATCH 4 Or in fact sign = extractFloatx80Sign(res); quot = floatx80_to_int32(floatx80_abs(res->d), status); make_quotient(env, sign, quot); r~
On 01/01/2023 17:26, Laurent Vivier wrote: > Le 01/01/2023 à 15:43, Mark Cave-Ayland a écrit : >> This enables the quotient parameter to be changed from int32_t to uint32_t and >> also allows the extra sign logic in make_quotient() to be removed. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> target/m68k/fpu_helper.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c >> index 0932c464fd..ae839785fa 100644 >> --- a/target/m68k/fpu_helper.c >> +++ b/target/m68k/fpu_helper.c >> @@ -515,15 +515,8 @@ uint32_t HELPER(fmovemd_ld_postinc)(CPUM68KState *env, >> uint32_t addr, >> return fmovem_postinc(env, addr, mask, cpu_ld_float64_ra); >> } >> -static void make_quotient(CPUM68KState *env, int32_t quotient) >> +static void make_quotient(CPUM68KState *env, int sign, uint32_t quotient) >> { >> - int sign; >> - >> - sign = quotient < 0; >> - if (sign) { >> - quotient = -quotient; >> - } >> - >> quotient = (sign << 7) | (quotient & 0x7f); >> env->fpsr = (env->fpsr & ~FPSR_QT_MASK) | (quotient << FPSR_QT_SHIFT); >> } >> @@ -536,7 +529,8 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, >> FPReg *val1) >> return; >> } >> - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); >> + make_quotient(env, extractFloatx80Sign(res->d), >> + floatx80_to_int32(res->d, &env->fp_status)); >> } >> void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) >> @@ -547,7 +541,8 @@ void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, >> FPReg *val1) >> return; >> } >> - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); >> + make_quotient(env, extractFloatx80Sign(res->d), >> + floatx80_to_int32(res->d, &env->fp_status)); >> } >> void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val) > > I think you need an "abs(floatx80_to_int32())" in both cases as you do in PATCH 4 Ah yes that's probably true - I suspect I didn't notice because the static tests fail immediately until patches 3 and 4. ATB, Mark.
On 01/01/2023 18:53, Richard Henderson wrote: > On 1/1/23 09:26, Laurent Vivier wrote: >> Le 01/01/2023 à 15:43, Mark Cave-Ayland a écrit : >>> This enables the quotient parameter to be changed from int32_t to uint32_t and >>> also allows the extra sign logic in make_quotient() to be removed. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> target/m68k/fpu_helper.c | 15 +++++---------- >>> 1 file changed, 5 insertions(+), 10 deletions(-) >>> >>> diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c >>> index 0932c464fd..ae839785fa 100644 >>> --- a/target/m68k/fpu_helper.c >>> +++ b/target/m68k/fpu_helper.c >>> @@ -515,15 +515,8 @@ uint32_t HELPER(fmovemd_ld_postinc)(CPUM68KState *env, >>> uint32_t addr, >>> return fmovem_postinc(env, addr, mask, cpu_ld_float64_ra); >>> } >>> -static void make_quotient(CPUM68KState *env, int32_t quotient) >>> +static void make_quotient(CPUM68KState *env, int sign, uint32_t quotient) >>> { >>> - int sign; >>> - >>> - sign = quotient < 0; >>> - if (sign) { >>> - quotient = -quotient; >>> - } >>> - >>> quotient = (sign << 7) | (quotient & 0x7f); >>> env->fpsr = (env->fpsr & ~FPSR_QT_MASK) | (quotient << FPSR_QT_SHIFT); >>> } >>> @@ -536,7 +529,8 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, >>> FPReg *val1) >>> return; >>> } >>> - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); >>> + make_quotient(env, extractFloatx80Sign(res->d), >>> + floatx80_to_int32(res->d, &env->fp_status)); >>> } >>> void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) >>> @@ -547,7 +541,8 @@ void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, >>> FPReg *val1) >>> return; >>> } >>> - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); >>> + make_quotient(env, extractFloatx80Sign(res->d), >>> + floatx80_to_int32(res->d, &env->fp_status)); >>> } >>> void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val) >> >> I think you need an "abs(floatx80_to_int32())" in both cases as you do in PATCH 4 > > Or in fact > > sign = extractFloatx80Sign(res); > quot = floatx80_to_int32(floatx80_abs(res->d), status); > make_quotient(env, sign, quot); Thanks for the suggestion. Just out of curiosity, how does moving the abs to before the integer conversion make a difference here? Is it because floatx80_to_int32() can fail in some circumstances because of the sign of the result? ATB, Mark.
On 1/2/23 02:10, Mark Cave-Ayland wrote: >>> I think you need an "abs(floatx80_to_int32())" in both cases as you do in PATCH 4 >> >> Or in fact >> >> sign = extractFloatx80Sign(res); >> quot = floatx80_to_int32(floatx80_abs(res->d), status); >> make_quotient(env, sign, quot); > > Thanks for the suggestion. Just out of curiosity, how does moving the abs to before the > integer conversion make a difference here? Is it because floatx80_to_int32() can fail in > some circumstances because of the sign of the result? It's a simple and operation on floats, instead of cmp+cmov on integers. I think it's a touch clearer as well, having just saved the fp sign, you discard it before moving on to the next thing. r~
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c index 0932c464fd..ae839785fa 100644 --- a/target/m68k/fpu_helper.c +++ b/target/m68k/fpu_helper.c @@ -515,15 +515,8 @@ uint32_t HELPER(fmovemd_ld_postinc)(CPUM68KState *env, uint32_t addr, return fmovem_postinc(env, addr, mask, cpu_ld_float64_ra); } -static void make_quotient(CPUM68KState *env, int32_t quotient) +static void make_quotient(CPUM68KState *env, int sign, uint32_t quotient) { - int sign; - - sign = quotient < 0; - if (sign) { - quotient = -quotient; - } - quotient = (sign << 7) | (quotient & 0x7f); env->fpsr = (env->fpsr & ~FPSR_QT_MASK) | (quotient << FPSR_QT_SHIFT); } @@ -536,7 +529,8 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) return; } - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); + make_quotient(env, extractFloatx80Sign(res->d), + floatx80_to_int32(res->d, &env->fp_status)); } void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) @@ -547,7 +541,8 @@ void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1) return; } - make_quotient(env, floatx80_to_int32(res->d, &env->fp_status)); + make_quotient(env, extractFloatx80Sign(res->d), + floatx80_to_int32(res->d, &env->fp_status)); } void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val)
This enables the quotient parameter to be changed from int32_t to uint32_t and also allows the extra sign logic in make_quotient() to be removed. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- target/m68k/fpu_helper.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)