From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by sourceware.org (Postfix) with ESMTPS id 4EF0B385702C for ; Tue, 4 Aug 2020 14:55:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4EF0B385702C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x441.google.com with SMTP id l2so27157966wrc.7 for ; Tue, 04 Aug 2020 07:55:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=I/Nmkrv0Plf+8mynV/ch5HYS4uuf3aj7eOEpuBpY6jk=; b=BIkgC+L+YgUa8VRm3OgujbCTwA23D4Fh3Tx2cbpe2ajq2Dh8sOvyWXM4W7YDHl+W4D B7Rgy+ANncnfZQx1AT7gebLu+Ez/yvjac2KJaT9RQkDTgcqGZ7rS3b92X3WgnmCCwgKf dhd+KImKboteHepxrqtuPXlISzOn+urbMrDymiFwdySZjZ5g245ImINyTX5NuGO2ZYVD NbC64hjr7wILMqe/h0Dpms/Sa2flNfqjQ8DpGj03TG7EurKoxkueWVGCLtdo7DtYOAqD goaQjDG+5DZ46VvHrWli4kxFOzSWrRu6yAkxLnQp7ZAswBVODwniGgJb1Gga/VLmiBg5 ixcg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=I/Nmkrv0Plf+8mynV/ch5HYS4uuf3aj7eOEpuBpY6jk=; b=c3wKflY8qJogdScwd6CKGWfuV+9yC14Sp0g3LGdYqt6/zAKW0AzADqFjJNO8gf6Dld uQzEjxx6Oylry/caRGhogs+aqGG/WEVPnP+Obn64zuOVuCGszMHy3DIPS9WnM/5KX7u6 /Sghlm+EvnHfxB0EmsWRLE+YHxFJsLx66xNLQ1jgsGuTincOXn8GnbdlOVgEmCjFXppg n2gl+tiY4BxewfUJ5s8/nhlhcMf/zu2zNULDG9zk+6hF9YUmxJbN9eXeiSurobdbA4cI Ff4S5TiHWkQZfaQ77X+7A+YG6T0BThCmnI32eVajQJMCOsiXJKobS+LuQef6AYQRN9aE ueDA== X-Gm-Message-State: AOAM533kXdG8ue/IpbMblOiXR9E61g+n/4ukchN7/uUOLJTqIPJ/yCJc O/VQ7t8T37sDzYsxehEiCux/oEnc6Jw= X-Google-Smtp-Source: ABdhPJy4WnQhjNYoNOkMk5DOO+sqI77O03k/dnCkxJ38Pp1mJY4qKzvUeAfuFogh36iccARhd8i4Lw== X-Received: by 2002:adf:ab55:: with SMTP id r21mr18971480wrc.332.1596552939346; Tue, 04 Aug 2020 07:55:39 -0700 (PDT) Received: from localhost (host86-140-161-92.range86-140.btcentralplus.com. [86.140.161.92]) by smtp.gmail.com with ESMTPSA id s20sm4664813wmh.21.2020.08.04.07.55.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Aug 2020 07:55:37 -0700 (PDT) Date: Tue, 4 Aug 2020 15:55:36 +0100 From: Andrew Burgess To: Jozef Lawrynowicz Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] MSP430: sim: Fix incorrect simulation of unsigned widening multiply Message-ID: <20200804145536.GY853475@embecosm.com> References: <20200731141100.adrjos6aixi65r2y@jozef-acer-manjaro> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200731141100.adrjos6aixi65r2y@jozef-acer-manjaro> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 15:55:22 up 17 days, 9 min, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Aug 2020 14:55:42 -0000 * Jozef Lawrynowicz [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 > 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 > > * 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 >