From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47127) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9pBX-0002pB-Ia for qemu-devel@nongnu.org; Tue, 12 Apr 2011 21:48:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q9lCF-0008Cx-Ec for qemu-devel@nongnu.org; Tue, 12 Apr 2011 17:32:28 -0400 Received: from hall.aurel32.net ([88.191.126.93]:43136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9lCF-0008CH-6G for qemu-devel@nongnu.org; Tue, 12 Apr 2011 17:32:27 -0400 Date: Tue, 12 Apr 2011 23:32:15 +0200 From: Aurelien Jarno Message-ID: <20110412213215.GA14070@volta.aurel32.net> References: <1302535928-15901-1-git-send-email-peter.maydell@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <1302535928-15901-1-git-send-email-peter.maydell@linaro.org> Subject: Re: [Qemu-devel] [PATCH] target-arm: Don't overflow when calculating value for signed VABAL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, patches@linaro.org On Mon, Apr 11, 2011 at 04:32:08PM +0100, Peter Maydell wrote: > In the VABAL instruction we take the absolute difference of two > values of size x and store it in a result of size 2x. This means > we have to be careful to calculate the absolute difference using > a wide enough type that we don't accidentally overflow. > > Signed-off-by: Peter Maydell > --- > target-arm/neon_helper.c | 38 +++++++++++++++++++++----------------- > 1 files changed, 21 insertions(+), 17 deletions(-) > > diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c > index c3ac96a..7df925a 100644 > --- a/target-arm/neon_helper.c > +++ b/target-arm/neon_helper.c > @@ -1514,9 +1514,13 @@ uint64_t HELPER(neon_addl_saturate_s64)(uint64_t a, uint64_t b) > return result; > } > > -#define DO_ABD(dest, x, y, type) do { \ > - type tmp_x = x; \ > - type tmp_y = y; \ > +/* We have to do the arithmetic in a larger type than > + * the input type, because for example with a signed 32 bit > + * op the absolute difference can overflow a signed 32 bit value. > + */ > +#define DO_ABD(dest, x, y, intype, arithtype) do { \ > + arithtype tmp_x = (intype)(x); \ > + arithtype tmp_y = (intype)(y); \ > dest = ((tmp_x > tmp_y) ? tmp_x - tmp_y : tmp_y - tmp_x); \ > } while(0) > > @@ -1524,12 +1528,12 @@ uint64_t HELPER(neon_abdl_u16)(uint32_t a, uint32_t b) > { > uint64_t tmp; > uint64_t result; > - DO_ABD(result, a, b, uint8_t); > - DO_ABD(tmp, a >> 8, b >> 8, uint8_t); > + DO_ABD(result, a, b, uint8_t, uint32_t); > + DO_ABD(tmp, a >> 8, b >> 8, uint8_t, uint32_t); > result |= tmp << 16; > - DO_ABD(tmp, a >> 16, b >> 16, uint8_t); > + DO_ABD(tmp, a >> 16, b >> 16, uint8_t, uint32_t); > result |= tmp << 32; > - DO_ABD(tmp, a >> 24, b >> 24, uint8_t); > + DO_ABD(tmp, a >> 24, b >> 24, uint8_t, uint32_t); > result |= tmp << 48; > return result; > } Do we really need a 32-bit type for the computation here? > @@ -1538,12 +1542,12 @@ uint64_t HELPER(neon_abdl_s16)(uint32_t a, uint32_t b) > { > uint64_t tmp; > uint64_t result; > - DO_ABD(result, a, b, int8_t); > - DO_ABD(tmp, a >> 8, b >> 8, int8_t); > + DO_ABD(result, a, b, int8_t, int32_t); > + DO_ABD(tmp, a >> 8, b >> 8, int8_t, int32_t); > result |= tmp << 16; > - DO_ABD(tmp, a >> 16, b >> 16, int8_t); > + DO_ABD(tmp, a >> 16, b >> 16, int8_t, int32_t); > result |= tmp << 32; > - DO_ABD(tmp, a >> 24, b >> 24, int8_t); > + DO_ABD(tmp, a >> 24, b >> 24, int8_t, int32_t); > result |= tmp << 48; > return result; > } Ditto. > @@ -1552,8 +1556,8 @@ uint64_t HELPER(neon_abdl_u32)(uint32_t a, uint32_t b) > { > uint64_t tmp; > uint64_t result; > - DO_ABD(result, a, b, uint16_t); > - DO_ABD(tmp, a >> 16, b >> 16, uint16_t); > + DO_ABD(result, a, b, uint16_t, uint32_t); > + DO_ABD(tmp, a >> 16, b >> 16, uint16_t, uint32_t); > return result | (tmp << 32); > } > > @@ -1561,22 +1565,22 @@ uint64_t HELPER(neon_abdl_s32)(uint32_t a, uint32_t b) > { > uint64_t tmp; > uint64_t result; > - DO_ABD(result, a, b, int16_t); > - DO_ABD(tmp, a >> 16, b >> 16, int16_t); > + DO_ABD(result, a, b, int16_t, int32_t); > + DO_ABD(tmp, a >> 16, b >> 16, int16_t, int32_t); > return result | (tmp << 32); > } > > uint64_t HELPER(neon_abdl_u64)(uint32_t a, uint32_t b) > { > uint64_t result; > - DO_ABD(result, a, b, uint32_t); > + DO_ABD(result, a, b, uint32_t, uint64_t); > return result; > } > > uint64_t HELPER(neon_abdl_s64)(uint32_t a, uint32_t b) > { > uint64_t result; > - DO_ABD(result, a, b, int32_t); > + DO_ABD(result, a, b, int32_t, int64_t); > return result; > } > #undef DO_ABD All the others looks fine. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net