From: Andrew Burgess <andrew.burgess@embecosm.com>
To: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] MSP430: sim: Fix incorrect simulation of unsigned widening multiply
Date: Tue, 4 Aug 2020 15:55:36 +0100 [thread overview]
Message-ID: <20200804145536.GY853475@embecosm.com> (raw)
In-Reply-To: <20200731141100.adrjos6aixi65r2y@jozef-acer-manjaro>
* Jozef Lawrynowicz <jozef.l@mittosystems.com> [2020-07-31 15:11:00 +0100]:
> Operand sizes used for simulation of MSP430 hardware multiply
> operations are not aligned with the sizes used on the target, resulting
> in the simulator storing signed operands with too much precision.
>
> Additionally, simulation of unsigned multiplication is missing explicit
> casts to prevent any implicit sign extension.
>
> gcc.c-torture/execute/pr91450-1.c uses unsigned widening multiplication
> of 32-bit operands -4 and 2, to a 64-bit result:
> 0xffff fffc * 0x2 = 0x1 ffff fff8
>
> If -4 is stored in 64-bit precision, then the multiplication is
> essentially signed and the result is -8 in 64-bit precision
> (0xffff ffff ffff fffc), which is not correct.
>
> Successfully regtested the GCC and G++ DejaGNU testsuites for msp430-elf in the
> default configuration, and with -mhwmult=f5series.
> This patch fixes 158 execution failures for -mhwmult=f5series.
>
> Ok to apply?
Approved.
Thanks,
Andrew
>
> Thanks,
> Jozef
> From 68a3cab29e116dc91a07add7ee16d19175db36a1 Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> Date: Tue, 28 Jul 2020 10:36:10 +0100
> Subject: [PATCH] MSP430: sim: Fix incorrect simulation of unsigned widening
> multiply
>
> Operand sizes used for simulation of MSP430 hardware multiply
> operations are not aligned with the sizes used on the target, resulting
> in the simulator storing signed operands with too much precision.
>
> Additionally, simulation of unsigned multiplication is missing explicit
> casts to prevent any implicit sign extension.
>
> gcc.c-torture/execute/pr91450-1.c uses unsigned widening multiplication
> of 32-bit operands -4 and 2, to a 64-bit result:
> 0xffff fffc * 0x2 = 0x1 ffff fff8
>
> If -4 is stored in 64-bit precision, then the multiplication is
> essentially signed and the result is -8 in 64-bit precision
> (0xffff ffff ffff fffc), which is not correct.
>
> sim/msp430/ChangeLog:
>
> 2020-07-31 Jozef Lawrynowicz <jozef.l@mittosystems.com>
>
> * msp430-sim.c (put_op): For unsigned multiplication, explicitly cast
> operands to the unsigned type before multiplying.
> * msp430-sim.h (struct msp430_cpu_state): Fix types used to store hwmult
> operands.
> ---
> sim/msp430/msp430-sim.c | 28 ++++++++++++++++++++--------
> sim/msp430/msp430-sim.h | 8 ++++----
> 2 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
> index e21c8cf6a64..a330c6caf5d 100644
> --- a/sim/msp430/msp430-sim.c
> +++ b/sim/msp430/msp430-sim.c
> @@ -566,8 +566,13 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
> switch (HWMULT (sd, hwmult_type))
> {
> case UNSIGN_32:
> - HWMULT (sd, hwmult_result) = HWMULT (sd, hwmult_op1) * HWMULT (sd, hwmult_op2);
> - HWMULT (sd, hwmult_signed_result) = (signed) HWMULT (sd, hwmult_result);
> + a = HWMULT (sd, hwmult_op1);
> + b = HWMULT (sd, hwmult_op2);
> + /* For unsigned 32-bit multiplication of 16-bit operands, an
> + explicit cast is required to prevent any implicit
> + sign-extension. */
> + HWMULT (sd, hwmult_result) = (unsigned32) a * (unsigned32) b;
> + HWMULT (sd, hwmult_signed_result) = a * b;
> HWMULT (sd, hwmult_accumulator) = HWMULT (sd, hwmult_signed_accumulator) = 0;
> break;
>
> @@ -575,13 +580,16 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
> a = sign_ext (HWMULT (sd, hwmult_op1), 16);
> b = sign_ext (HWMULT (sd, hwmult_op2), 16);
> HWMULT (sd, hwmult_signed_result) = a * b;
> - HWMULT (sd, hwmult_result) = (unsigned) HWMULT (sd, hwmult_signed_result);
> + HWMULT (sd, hwmult_result) = (unsigned32) a * (unsigned32) b;
> HWMULT (sd, hwmult_accumulator) = HWMULT (sd, hwmult_signed_accumulator) = 0;
> break;
>
> case UNSIGN_MAC_32:
> - HWMULT (sd, hwmult_accumulator) += HWMULT (sd, hwmult_op1) * HWMULT (sd, hwmult_op2);
> - HWMULT (sd, hwmult_signed_accumulator) += HWMULT (sd, hwmult_op1) * HWMULT (sd, hwmult_op2);
> + a = HWMULT (sd, hwmult_op1);
> + b = HWMULT (sd, hwmult_op2);
> + HWMULT (sd, hwmult_accumulator)
> + += (unsigned32) a * (unsigned32) b;
> + HWMULT (sd, hwmult_signed_accumulator) += a * b;
> HWMULT (sd, hwmult_result) = HWMULT (sd, hwmult_accumulator);
> HWMULT (sd, hwmult_signed_result) = HWMULT (sd, hwmult_signed_accumulator);
> break;
> @@ -589,7 +597,8 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
> case SIGN_MAC_32:
> a = sign_ext (HWMULT (sd, hwmult_op1), 16);
> b = sign_ext (HWMULT (sd, hwmult_op2), 16);
> - HWMULT (sd, hwmult_accumulator) += a * b;
> + HWMULT (sd, hwmult_accumulator)
> + += (unsigned32) a * (unsigned32) b;
> HWMULT (sd, hwmult_signed_accumulator) += a * b;
> HWMULT (sd, hwmult_result) = HWMULT (sd, hwmult_accumulator);
> HWMULT (sd, hwmult_signed_result) = HWMULT (sd, hwmult_signed_accumulator);
> @@ -648,10 +657,13 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
> switch (HWMULT (sd, hw32mult_type))
> {
> case UNSIGN_64:
> - HWMULT (sd, hw32mult_result) = HWMULT (sd, hw32mult_op1) * HWMULT (sd, hw32mult_op2);
> + HWMULT (sd, hw32mult_result)
> + = (unsigned64) HWMULT (sd, hw32mult_op1)
> + * (unsigned64) HWMULT (sd, hw32mult_op2);
> break;
> case SIGN_64:
> - HWMULT (sd, hw32mult_result) = sign_ext (HWMULT (sd, hw32mult_op1), 32)
> + HWMULT (sd, hw32mult_result)
> + = sign_ext (HWMULT (sd, hw32mult_op1), 32)
> * sign_ext (HWMULT (sd, hw32mult_op2), 32);
> break;
> }
> diff --git a/sim/msp430/msp430-sim.h b/sim/msp430/msp430-sim.h
> index ad83e5b6ae6..7c486c2f350 100644
> --- a/sim/msp430/msp430-sim.h
> +++ b/sim/msp430/msp430-sim.h
> @@ -31,16 +31,16 @@ struct msp430_cpu_state
> int cio_buffer;
>
> hwmult_type hwmult_type;
> - unsigned32 hwmult_op1;
> - unsigned32 hwmult_op2;
> + unsigned16 hwmult_op1;
> + unsigned16 hwmult_op2;
> unsigned32 hwmult_result;
> signed32 hwmult_signed_result;
> unsigned32 hwmult_accumulator;
> signed32 hwmult_signed_accumulator;
>
> hw32mult_type hw32mult_type;
> - unsigned64 hw32mult_op1;
> - unsigned64 hw32mult_op2;
> + unsigned32 hw32mult_op1;
> + unsigned32 hw32mult_op2;
> unsigned64 hw32mult_result;
> };
>
> --
> 2.27.0
>
prev parent reply other threads:[~2020-08-04 14:55 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-31 14:11 Jozef Lawrynowicz
2020-08-04 14:55 ` Andrew Burgess [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200804145536.GY853475@embecosm.com \
--to=andrew.burgess@embecosm.com \
--cc=gdb-patches@sourceware.org \
--cc=jozef.l@mittosystems.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox