* [PATCH v2 1/4] target/m68k: pass quotient directly into make_quotient()
2023-01-04 13:45 [PATCH v2 0/4] target/m68k: fix FPSR quotient byte for fmod and frem Mark Cave-Ayland
@ 2023-01-04 13:45 ` Mark Cave-Ayland
2023-01-04 17:37 ` Richard Henderson
2023-01-04 13:45 ` [PATCH v2 2/4] target/m68k: pass sign " Mark Cave-Ayland
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Mark Cave-Ayland @ 2023-01-04 13:45 UTC (permalink / raw)
To: laurent, qemu-devel
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
target/m68k/fpu_helper.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index fdc4937e29..0932c464fd 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -515,16 +515,10 @@ 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, floatx80 val)
+static void make_quotient(CPUM68KState *env, int32_t quotient)
{
- int32_t quotient;
int sign;
- if (floatx80_is_any_nan(val)) {
- return;
- }
-
- quotient = floatx80_to_int32(val, &env->fp_status);
sign = quotient < 0;
if (sign) {
quotient = -quotient;
@@ -538,14 +532,22 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
{
res->d = floatx80_mod(val1->d, val0->d, &env->fp_status);
- make_quotient(env, res->d);
+ if (floatx80_is_any_nan(res->d)) {
+ return;
+ }
+
+ make_quotient(env, floatx80_to_int32(res->d, &env->fp_status));
}
void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
{
res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
- make_quotient(env, res->d);
+ if (floatx80_is_any_nan(res->d)) {
+ return;
+ }
+
+ make_quotient(env, floatx80_to_int32(res->d, &env->fp_status));
}
void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val)
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] target/m68k: pass sign directly into make_quotient()
2023-01-04 13:45 [PATCH v2 0/4] target/m68k: fix FPSR quotient byte for fmod and frem Mark Cave-Ayland
2023-01-04 13:45 ` [PATCH v2 1/4] target/m68k: pass quotient directly into make_quotient() Mark Cave-Ayland
@ 2023-01-04 13:45 ` Mark Cave-Ayland
2023-01-04 16:04 ` Laurent Vivier
2023-01-04 17:38 ` Richard Henderson
2023-01-04 13:45 ` [PATCH v2 3/4] target/m68k: fix FPSR quotient byte for fmod instruction Mark Cave-Ayland
2023-01-04 13:45 ` [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction Mark Cave-Ayland
3 siblings, 2 replies; 12+ messages in thread
From: Mark Cave-Ayland @ 2023-01-04 13:45 UTC (permalink / raw)
To: laurent, qemu-devel
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 | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 0932c464fd..76b34b8988 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -515,39 +515,42 @@ 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);
}
void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
{
+ uint32_t quotient;
+ int sign;
+
res->d = floatx80_mod(val1->d, val0->d, &env->fp_status);
if (floatx80_is_any_nan(res->d)) {
return;
}
- make_quotient(env, floatx80_to_int32(res->d, &env->fp_status));
+ sign = extractFloatx80Sign(res->d);
+ quotient = floatx80_to_int32(floatx80_abs(res->d), &env->fp_status);
+ make_quotient(env, sign, quotient);
}
void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
{
+ uint32_t quotient;
+ int sign;
+
res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
if (floatx80_is_any_nan(res->d)) {
return;
}
- make_quotient(env, floatx80_to_int32(res->d, &env->fp_status));
+ sign = extractFloatx80Sign(res->d);
+ quotient = floatx80_to_int32(floatx80_abs(res->d), &env->fp_status);
+ make_quotient(env, sign, quotient);
}
void HELPER(fgetexp)(CPUM68KState *env, FPReg *res, FPReg *val)
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] target/m68k: pass sign directly into make_quotient()
2023-01-04 13:45 ` [PATCH v2 2/4] target/m68k: pass sign " Mark Cave-Ayland
@ 2023-01-04 16:04 ` Laurent Vivier
2023-01-04 17:38 ` Richard Henderson
1 sibling, 0 replies; 12+ messages in thread
From: Laurent Vivier @ 2023-01-04 16:04 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
Le 04/01/2023 à 14:45, 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 | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] target/m68k: pass sign directly into make_quotient()
2023-01-04 13:45 ` [PATCH v2 2/4] target/m68k: pass sign " Mark Cave-Ayland
2023-01-04 16:04 ` Laurent Vivier
@ 2023-01-04 17:38 ` Richard Henderson
1 sibling, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2023-01-04 17:38 UTC (permalink / raw)
To: Mark Cave-Ayland, laurent, qemu-devel
On 1/4/23 05:45, Mark Cave-Ayland wrote:
> 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 | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] target/m68k: fix FPSR quotient byte for fmod instruction
2023-01-04 13:45 [PATCH v2 0/4] target/m68k: fix FPSR quotient byte for fmod and frem Mark Cave-Ayland
2023-01-04 13:45 ` [PATCH v2 1/4] target/m68k: pass quotient directly into make_quotient() Mark Cave-Ayland
2023-01-04 13:45 ` [PATCH v2 2/4] target/m68k: pass sign " Mark Cave-Ayland
@ 2023-01-04 13:45 ` Mark Cave-Ayland
2023-01-04 17:59 ` Richard Henderson
2023-01-04 13:45 ` [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction Mark Cave-Ayland
3 siblings, 1 reply; 12+ messages in thread
From: Mark Cave-Ayland @ 2023-01-04 13:45 UTC (permalink / raw)
To: laurent, qemu-devel
The FPSR quotient byte should be set to the value of the quotient and not the
result. Switch from using floatx80_mod() to floatx80_modrem() which returns
the quotient as a uint64_t which can be used for the quotient byte.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
target/m68k/fpu_helper.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 76b34b8988..5fd094a33c 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -523,17 +523,16 @@ static void make_quotient(CPUM68KState *env, int sign, uint32_t quotient)
void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
{
- uint32_t quotient;
- int sign;
+ uint64_t quotient;
+ int sign = extractFloatx80Sign(val1->d) ^ extractFloatx80Sign(val0->d);
- res->d = floatx80_mod(val1->d, val0->d, &env->fp_status);
+ res->d = floatx80_modrem(val1->d, val0->d, true, "ient,
+ &env->fp_status);
if (floatx80_is_any_nan(res->d)) {
return;
}
- sign = extractFloatx80Sign(res->d);
- quotient = floatx80_to_int32(floatx80_abs(res->d), &env->fp_status);
make_quotient(env, sign, quotient);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] target/m68k: fix FPSR quotient byte for fmod instruction
2023-01-04 13:45 ` [PATCH v2 3/4] target/m68k: fix FPSR quotient byte for fmod instruction Mark Cave-Ayland
@ 2023-01-04 17:59 ` Richard Henderson
0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2023-01-04 17:59 UTC (permalink / raw)
To: Mark Cave-Ayland, laurent, qemu-devel
On 1/4/23 05:45, Mark Cave-Ayland wrote:
> The FPSR quotient byte should be set to the value of the quotient and not the
> result. Switch from using floatx80_mod() to floatx80_modrem() which returns
> the quotient as a uint64_t which can be used for the quotient byte.
>
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laurent Vivier<laurent@vivier.eu>
> ---
> target/m68k/fpu_helper.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction
2023-01-04 13:45 [PATCH v2 0/4] target/m68k: fix FPSR quotient byte for fmod and frem Mark Cave-Ayland
` (2 preceding siblings ...)
2023-01-04 13:45 ` [PATCH v2 3/4] target/m68k: fix FPSR quotient byte for fmod instruction Mark Cave-Ayland
@ 2023-01-04 13:45 ` Mark Cave-Ayland
2023-01-04 18:04 ` Richard Henderson
3 siblings, 1 reply; 12+ messages in thread
From: Mark Cave-Ayland @ 2023-01-04 13:45 UTC (permalink / raw)
To: laurent, qemu-devel
The FPSR quotient byte should be set to the value of the quotient and not the
result. Manually calculate the quotient in the frem helper in round to nearest
even mode (note this is different from the quotient calculated internally for
fmod), and use it to set the quotient byte accordingly.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1314
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
target/m68k/fpu_helper.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 5fd094a33c..56f7400140 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -538,17 +538,25 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
{
+ float_status fp_status;
+ FPReg fp_quot;
uint32_t quotient;
int sign;
+ /* Calculate quotient directly using round to nearest mode */
+ set_float_rounding_mode(float_round_nearest_even, &fp_status);
+ set_floatx80_rounding_precision(
+ get_floatx80_rounding_precision(&env->fp_status), &fp_status);
+ fp_quot.d = floatx80_div(val1->d, val0->d, &fp_status);
+
res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
- if (floatx80_is_any_nan(res->d)) {
+ if (floatx80_is_any_nan(fp_quot.d)) {
return;
}
- sign = extractFloatx80Sign(res->d);
- quotient = floatx80_to_int32(floatx80_abs(res->d), &env->fp_status);
+ sign = extractFloatx80Sign(fp_quot.d);
+ quotient = floatx80_to_int32(floatx80_abs(fp_quot.d), &env->fp_status);
make_quotient(env, sign, quotient);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction
2023-01-04 13:45 ` [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction Mark Cave-Ayland
@ 2023-01-04 18:04 ` Richard Henderson
2023-01-07 23:00 ` Mark Cave-Ayland
0 siblings, 1 reply; 12+ messages in thread
From: Richard Henderson @ 2023-01-04 18:04 UTC (permalink / raw)
To: Mark Cave-Ayland, laurent, qemu-devel
On 1/4/23 05:45, Mark Cave-Ayland wrote:
> The FPSR quotient byte should be set to the value of the quotient and not the
> result. Manually calculate the quotient in the frem helper in round to nearest
> even mode (note this is different from the quotient calculated internally for
> fmod), and use it to set the quotient byte accordingly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1314
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> ---
> target/m68k/fpu_helper.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
> index 5fd094a33c..56f7400140 100644
> --- a/target/m68k/fpu_helper.c
> +++ b/target/m68k/fpu_helper.c
> @@ -538,17 +538,25 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
>
> void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
> {
> + float_status fp_status;
> + FPReg fp_quot;
> uint32_t quotient;
> int sign;
>
> + /* Calculate quotient directly using round to nearest mode */
> + set_float_rounding_mode(float_round_nearest_even, &fp_status);
> + set_floatx80_rounding_precision(
> + get_floatx80_rounding_precision(&env->fp_status), &fp_status);
> + fp_quot.d = floatx80_div(val1->d, val0->d, &fp_status);
> +
> res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
>
> - if (floatx80_is_any_nan(res->d)) {
> + if (floatx80_is_any_nan(fp_quot.d)) {
I think you should leave this line unchanged, and move the div afterward.
I also think you should completely initialize the local fp_status = { }.
With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction
2023-01-04 18:04 ` Richard Henderson
@ 2023-01-07 23:00 ` Mark Cave-Ayland
2023-01-09 1:55 ` Richard Henderson
0 siblings, 1 reply; 12+ messages in thread
From: Mark Cave-Ayland @ 2023-01-07 23:00 UTC (permalink / raw)
To: Richard Henderson, laurent, qemu-devel
On 04/01/2023 18:04, Richard Henderson wrote:
> On 1/4/23 05:45, Mark Cave-Ayland wrote:
>> The FPSR quotient byte should be set to the value of the quotient and not the
>> result. Manually calculate the quotient in the frem helper in round to nearest
>> even mode (note this is different from the quotient calculated internally for
>> fmod), and use it to set the quotient byte accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1314
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>> target/m68k/fpu_helper.c | 14 +++++++++++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
>> index 5fd094a33c..56f7400140 100644
>> --- a/target/m68k/fpu_helper.c
>> +++ b/target/m68k/fpu_helper.c
>> @@ -538,17 +538,25 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0,
>> FPReg *val1)
>> void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
>> {
>> + float_status fp_status;
>> + FPReg fp_quot;
>> uint32_t quotient;
>> int sign;
>> + /* Calculate quotient directly using round to nearest mode */
>> + set_float_rounding_mode(float_round_nearest_even, &fp_status);
>> + set_floatx80_rounding_precision(
>> + get_floatx80_rounding_precision(&env->fp_status), &fp_status);
>> + fp_quot.d = floatx80_div(val1->d, val0->d, &fp_status);
>> +
>> res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
>> - if (floatx80_is_any_nan(res->d)) {
>> + if (floatx80_is_any_nan(fp_quot.d)) {
>
> I think you should leave this line unchanged, and move the div afterward.
> I also think you should completely initialize the local fp_status = { }.
>
> With that,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
I can leave the floatx80_is_any_nan() line above unchanged and also initialise the
local fp_status, however the floatx80_div() has to happen before floatx80_rem()
function is called. This is because the fmod and frem instructions write the result
back to one of the input registers, which then causes the subsequent floatx80_div()
to return an incorrect result.
Would just these 2 changes be enough to keep your R-B tag for a v3?
ATB,
Mark.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction
2023-01-07 23:00 ` Mark Cave-Ayland
@ 2023-01-09 1:55 ` Richard Henderson
0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2023-01-09 1:55 UTC (permalink / raw)
To: Mark Cave-Ayland, laurent, qemu-devel
On 1/7/23 15:00, Mark Cave-Ayland wrote:
>>> void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
>>> {
>>> + float_status fp_status;
>>> + FPReg fp_quot;
>>> uint32_t quotient;
>>> int sign;
>>> + /* Calculate quotient directly using round to nearest mode */
>>> + set_float_rounding_mode(float_round_nearest_even, &fp_status);
>>> + set_floatx80_rounding_precision(
>>> + get_floatx80_rounding_precision(&env->fp_status), &fp_status);
>>> + fp_quot.d = floatx80_div(val1->d, val0->d, &fp_status);
>>> +
>>> res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
>>> - if (floatx80_is_any_nan(res->d)) {
>>> + if (floatx80_is_any_nan(fp_quot.d)) {
>>
>> I think you should leave this line unchanged, and move the div afterward.
>> I also think you should completely initialize the local fp_status = { }.
>>
>> With that,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> I can leave the floatx80_is_any_nan() line above unchanged and also initialise the local
> fp_status, however the floatx80_div() has to happen before floatx80_rem() function is
> called. This is because the fmod and frem instructions write the result back to one of the
> input registers, which then causes the subsequent floatx80_div() to return an incorrect
> result.
>
> Would just these 2 changes be enough to keep your R-B tag for a v3?
Mm. I suppose so. Otherwise, compute into a local variable:
floatx80 fp_rem = floatx80_rem(val1->d, val0->d, &env->fp_status);
if (!floatx80_is_any_nan(fp_rem)) {
float_status scratch = env->fp_status;
floatx80 fp_quot;
uint32_t int_quot;
int sign;
set_float_rounding_mode(float_round_nearest_even, &scratch);
fp_quot = floatx80_div(val1->d, val0->d, &scratch);
sign = ...
int_quot = ...
...
}
res->d = fp_rem;
?
r~
^ permalink raw reply [flat|nested] 12+ messages in thread