| Message ID | 20240404162641.27528-2-zack@buhman.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] sh4: mac.l: implement saturation arithmetic logic | expand |
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
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 >
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 >
On 4/4/24 06:26, Zack Buhman 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 > --- > target/sh4/op_helper.c | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) Queued, thanks. r~
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; } void helper_macw(CPUSH4State *env, uint32_t arg0, uint32_t arg1)
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 --- target/sh4/op_helper.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)