Re: [PATCH v2] sh4: mac.l: implement saturation arithmetic logic

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 4 Apr 2024 at 17:26, Zack Buhman <zack@buhman.org> wrote:
>
> The saturation arithmetic logic in helper_macl is not correct.
>
> I tested and verified this behavior on a SH7091, the general pattern
> is a code sequence such as:
>
>         sets
>
>         mov.l _mach,r2
>         lds r2,mach
>         mov.l _macl,r2
>         lds r2,macl
>
>         mova _n,r0
>         mov r0,r1
>         mova _m,r0
>         mac.l @r0+,@r1+
>
>     _mach: .long 0x00007fff
>     _macl: .long 0x12345678
>     _m:    .long 0x7fffffff
>     _n:    .long 0x7fffffff
>
> Test case 0: (no int64_t overflow)
>   given; prior to saturation mac.l:
>     mach = 0x00007fff macl = 0x12345678
>     @r0  = 0x7fffffff @r1  = 0x7fffffff
>
>   expected saturation mac.l result:
>     mach = 0x00007fff macl = 0xffffffff
>
>   qemu saturation mac.l result (prior to this commit):
>     mach = 0x00007ffe macl = 0x12345678
>
> Test case 1: (no int64_t overflow)
>   given; prior to saturation mac.l:
>     mach = 0xffff8000 macl = 0x00000000
>     @r0  = 0xffffffff @r1  = 0x00000001
>
>   expected saturation mac.l result:
>     mach = 0xffff8000 macl = 0x00000000
>
>   qemu saturation mac.l result (prior to this commit):
>     mach = 0xffff7fff macl = 0xffffffff
>
> Test case 2: (int64_t addition overflow)
>   given; prior to saturation mac.l:
>     mach = 0x80000000 macl = 0x00000000
>     @r0  = 0xffffffff @r1  = 0x00000001
>
>   expected saturation mac.l result:
>     mach = 0xffff8000 macl = 0x00000000
>
>   qemu saturation mac.l result (prior to this commit):
>     mach = 0xffff7fff macl = 0xffffffff
>
> Test case 3: (int64_t addition overflow)
>   given; prior to saturation mac.l:
>     mach = 0x7fffffff macl = 0x00000000
>     @r0 = 0x7fffffff @r1 = 0x7fffffff
>
>   expected saturation mac.l result:
>     mach = 0x00007fff macl = 0xffffffff
>
>   qemu saturation mac.l result (prior to this commit):
>     mach = 0xfffffffe macl = 0x00000001
>
> All of the above also matches the description of MAC.L as documented
> in cd00147165-sh-4-32-bit-cpu-core-architecture-stmicroelectronics.pdf

Hi. I just noticed that you didn't include a signed-off-by line
in your commit message. We need these as they're how you say
that you're legally OK to contribute this code to QEMU and
you're happy for it to go into the project:

https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line
has links to what exactly this means, but basically the
requirement is that the last line of your commit message should be
"Signed-off-by: Your Name <your@email>"

In this case, if you just reply to this email with that, we
can pick it up and fix up the commit message when we apply the
patch.

> ---
>  target/sh4/op_helper.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/target/sh4/op_helper.c b/target/sh4/op_helper.c
> index 4559d0d376..ee16524083 100644
> --- a/target/sh4/op_helper.c
> +++ b/target/sh4/op_helper.c
> @@ -160,18 +160,29 @@ void helper_ocbi(CPUSH4State *env, uint32_t address)
>
>  void helper_macl(CPUSH4State *env, uint32_t arg0, uint32_t arg1)
>  {
> -    int64_t res;
> -
> -    res = ((uint64_t) env->mach << 32) | env->macl;
> -    res += (int64_t) (int32_t) arg0 *(int64_t) (int32_t) arg1;
> -    env->mach = (res >> 32) & 0xffffffff;
> -    env->macl = res & 0xffffffff;
> +    int32_t value0 = (int32_t)arg0;
> +    int32_t value1 = (int32_t)arg1;
> +    int64_t mul = ((int64_t)value0) * ((int64_t)value1);
> +    int64_t mac = (((uint64_t)env->mach) << 32) | env->macl;
> +    int64_t result;
> +    bool overflow = sadd64_overflow(mac, mul, &result);
> +    /* Perform 48-bit saturation arithmetic if the S flag is set */
>      if (env->sr & (1u << SR_S)) {
> -        if (res < 0)
> -            env->mach |= 0xffff0000;
> -        else
> -            env->mach &= 0x00007fff;
> +        /*
> +         * The sign bit of `mac + mul` may overflow. The MAC unit on
> +         * real SH-4 hardware has equivalent carry/saturation logic:
> +         */
> +        const int64_t upper_bound =  ((1ull << 47) - 1);
> +        const int64_t lower_bound = -((1ull << 47) - 0);
> +
> +        if (overflow) {
> +            result = (mac < 0) ? lower_bound : upper_bound;
> +        } else {
> +            result = MIN(MAX(result, lower_bound), upper_bound);
> +        }
>      }
> +    env->macl = result;
> +    env->mach = result >> 32;
>  }

I haven't checked the sh4 docs but the change looks right, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM