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

[Zack Buhman]

Signed-off-by: Zack Buhman <zack@buhman.org>

----- Original message -----
From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>, Zack Buhman <zack@buhman.org>
Cc: qemu-devel@nongnu.org, Yoshinori Sato <ysato@users.sourceforge.jp>
Subject: Re: [PATCH v2] sh4: mac.l: implement saturation arithmetic logic
Date: Friday, April 05, 2024 1:26 AM

Hi Zack,

Cc'ing the maintainer of this file, Yoshinori:

$ ./scripts/get_maintainer.pl -f target/sh4/op_helper.c
Yoshinori Sato <ysato@users.sourceforge.jp> (reviewer:SH4 TCG CPUs)
(https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer)

On 4/4/24 18:39, Peter Maydell wrote:
> 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
>