target/sparc: use signed denominator in sdiv helper

[PATCH] target/sparc: use signed denominator in sdiv helper

Posted by Clément Chigot 5 months, 3 weeks ago

The result has to be done with the signed denominator (b32) instead of
the unsigned value passed in argument (b).

Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 target/sparc/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index 2247e243b5..7846ddd6f6 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a, target_ulong b)
         return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
     }
 
-    a64 /= b;
+    a64 /= b32;
     r = a64;
     if (unlikely(r != a64)) {
         return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);
-- 
2.25.1

Re: [PATCH] target/sparc: use signed denominator in sdiv helper

Posted by Richard Henderson 5 months, 1 week ago

On 6/6/24 07:43, Clément Chigot wrote:
> The result has to be done with the signed denominator (b32) instead of
> the unsigned value passed in argument (b).
> 
> Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>   target/sparc/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/sparc/helper.c b/target/sparc/helper.c
> index 2247e243b5..7846ddd6f6 100644
> --- a/target/sparc/helper.c
> +++ b/target/sparc/helper.c
> @@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a, target_ulong b)
>           return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
>       }
>   
> -    a64 /= b;
> +    a64 /= b32;
>       r = a64;
>       if (unlikely(r != a64)) {
>           return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);

Queued, thanks.


r~

Re: [PATCH] target/sparc: use signed denominator in sdiv helper

Posted by Mark Cave-Ayland 5 months, 3 weeks ago

On 06/06/2024 15:43, Clément Chigot wrote:

> The result has to be done with the signed denominator (b32) instead of
> the unsigned value passed in argument (b).
> 
> Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>   target/sparc/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/sparc/helper.c b/target/sparc/helper.c
> index 2247e243b5..7846ddd6f6 100644
> --- a/target/sparc/helper.c
> +++ b/target/sparc/helper.c
> @@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a, target_ulong b)
>           return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
>       }
>   
> -    a64 /= b;
> +    a64 /= b32;
>       r = a64;
>       if (unlikely(r != a64)) {
>           return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);

Thanks for the patch! I think this might also be:

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2319


ATB,

Mark.

Re: [PATCH] target/sparc: use signed denominator in sdiv helper

Posted by Richard Henderson 5 months, 3 weeks ago

On 6/6/24 07:43, Clément Chigot wrote:
> The result has to be done with the signed denominator (b32) instead of
> the unsigned value passed in argument (b).
> 
> Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>   target/sparc/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/sparc/helper.c b/target/sparc/helper.c
> index 2247e243b5..7846ddd6f6 100644
> --- a/target/sparc/helper.c
> +++ b/target/sparc/helper.c
> @@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a, target_ulong b)
>           return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
>       }
>   
> -    a64 /= b;
> +    a64 /= b32;
>       r = a64;
>       if (unlikely(r != a64)) {
>           return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);

Oops.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~