[PATCH] MSP430: sim: Fix incorrect simulation of unsigned widening multiply

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] MSP430: sim: Fix incorrect simulation of unsigned widening multiply

Jozef Lawrynowicz-2
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?

Thanks,
Jozef

0001-MSP430-sim-Fix-incorrect-simulation-of-unsigned-wide.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] MSP430: sim: Fix incorrect simulation of unsigned widening multiply

Andrew Burgess
* Jozef Lawrynowicz <[hidden email]> [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 <[hidden email]>
> 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  <[hidden email]>
>
> * 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
>