Signed-off-by: Zack Buhman <[email protected]> ----- Original message ----- From: "Philippe Mathieu-Daudé" <[email protected]> To: Peter Maydell <[email protected]>, Zack Buhman <[email protected]> Cc: [email protected], Yoshinori Sato <[email protected]> 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 <[email protected]> (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 <[email protected]> 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 <[email protected]> > > thanks > -- PMM >
