Re: [PATCH v4 2/4] target/sh4: Fix SUBV opcode

[Yoshinori Sato]

On Wed, 01 May 2024 01:31:23 +0900,
Philippe Mathieu-Daudé wrote:
> 
> The documentation says:
> 
>   SUBV Rm, Rn        Rn - Rm -> Rn, underflow -> T
> 
> The overflow / underflow can be calculated as:
> 
>   T = ((Rn ^ Rm) & (Result ^ Rn)) >> 31
> 
> However we were using the incorrect:
> 
>   T = ((Rn ^ Rm) & (Result ^ Rm)) >> 31
> 
> Fix by using the Rn register instead of Rm.
> 
> Add tests provided by Paul Cercueil.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: ad8d25a11f ("target-sh4: implement addv and subv using TCG")
> Reported-by: Paul Cercueil <paul@crapouillou.net>
> Suggested-by: Paul Cercueil <paul@crapouillou.net>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2318
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/sh4/translate.c        |  2 +-
>  tests/tcg/sh4/test-subv.c     | 30 ++++++++++++++++++++++++++++++
>  tests/tcg/sh4/Makefile.target |  3 +++
>  3 files changed, 34 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/sh4/test-subv.c
> 
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 4a1dd0d1f4..3e013b7c7c 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -933,7 +933,7 @@ static void _decode_opc(DisasContext * ctx)
>              t0 = tcg_temp_new();
>              tcg_gen_sub_i32(t0, REG(B11_8), REG(B7_4));
>              t1 = tcg_temp_new();
> -            tcg_gen_xor_i32(t1, t0, REG(B7_4));
> +            tcg_gen_xor_i32(t1, t0, REG(B11_8));
>              t2 = tcg_temp_new();
>              tcg_gen_xor_i32(t2, REG(B11_8), REG(B7_4));
>              tcg_gen_and_i32(t1, t1, t2);
> diff --git a/tests/tcg/sh4/test-subv.c b/tests/tcg/sh4/test-subv.c
> new file mode 100644
> index 0000000000..0dd8fcdaac
> --- /dev/null
> +++ b/tests/tcg/sh4/test-subv.c
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <limits.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +static void subv(const int a, const int b, const int res, const int carry)
> +{
> +    int o = a, c;
> +
> +    asm volatile("subv %2,%0\n"
> +                 "movt %1\n"
> +                 : "+r"(o), "=r"(c) : "r"(b) :);
> +
> +    if (c != carry || o != res) {
> +        printf("SUBV %d, %d = %d/%d [T = %d/%d]\n", a, b, o, res, c, carry);
> +        abort();
> +    }
> +}
> +
> +int main(void)
> +{
> +    subv(INT_MIN, 1, INT_MAX, 1);
> +    subv(INT_MAX, -1, INT_MIN, 1);
> +    subv(INT_MAX, 1, INT_MAX - 1, 0);
> +    subv(0, 1, -1, 0);
> +    subv(-1, -1, 0, 0);
> +
> +    return 0;
> +}
> diff --git a/tests/tcg/sh4/Makefile.target b/tests/tcg/sh4/Makefile.target
> index 521b8b0a76..7852fa62d8 100644
> --- a/tests/tcg/sh4/Makefile.target
> +++ b/tests/tcg/sh4/Makefile.target
> @@ -20,3 +20,6 @@ TESTS += test-macw
>  
>  test-addv: CFLAGS += -O -g
>  TESTS += test-addv
> +
> +test-subv: CFLAGS += -O -g
> +TESTS += test-subv
> -- 
> 2.41.0
> 

Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>

-- 
Yosinori Sato