From patchwork Tue Mar 11 10:48:37 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Preud'homme X-Patchwork-Id: 329031 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id BBBBA2C00A1 for ; Tue, 11 Mar 2014 21:48:51 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:references:in-reply-to:subject:date:message-id:mime-version :content-type:content-transfer-encoding; q=dns; s=default; b=W0d TyqVeUyKY32v7wEhuS5DsIzmBPEdILAHiWqcFrUfLPBxJBs74WWKRx/1+tVWLJVs FriouqbSJNZ8ySfnalXDu4iAty+ovItCbjC4VF4KiDAI5t5aUYPhRzS+7WS+2ImQ +DjTrXzhQjvhqhmQk4SDk5qUhz6leX5pi5W/QeP4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:references:in-reply-to:subject:date:message-id:mime-version :content-type:content-transfer-encoding; s=default; bh=OXCo66u36 4fdXq3+HQpksKjkZc8=; b=br6BCVWZtQH2FEZ8x8RoYeOJHLD2W0ICzVd5OAwYJ 21VeSNXns5UMb7NnPlzJkUwp5qid+//V/18rxB/aT99HjdC/lTT1/jvVKr9/PnbS +IROLAWsyoCF2w3ffpr7arOT7eetb6ObvydZPzHWeU0OBS538r25Oo5RxB/9XW8B 9w= Received: (qmail 25628 invoked by alias); 11 Mar 2014 10:48:44 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 25611 invoked by uid 89); 11 Mar 2014 10:48:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: service87.mimecast.com Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Mar 2014 10:48:35 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Tue, 11 Mar 2014 10:48:32 +0000 Received: from SHAWIN202 ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Tue, 11 Mar 2014 10:48:39 +0000 From: "Thomas Preud'homme" To: References: <000301cf39e8$84eb6910$8ec23b30$@arm.com> <20140307141555.GJ22862@tucnak.redhat.com> <000001cf3cf6$a3ef8b00$ebcea100$@arm.com> <20140311095417.GY22862@tucnak.redhat.com> In-Reply-To: <20140311095417.GY22862@tucnak.redhat.com> Subject: RE: [PATCH] Fix incorrect byte swap detection (PR tree-optimization/60454) Date: Tue, 11 Mar 2014 18:48:37 +0800 Message-ID: <000401cf3d17$76a460a0$63ed21e0$@arm.com> MIME-Version: 1.0 X-MC-Unique: 114031110483201401 X-IsSubscribed: yes > From: Jakub Jelinek [mailto:jakub@redhat.com] > > The ChangeLog entry is wrong, the file is new, so you should just say: > * gcc.c-torture/execute/pr60454.c: New test. Done. > > But more importantly: > > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c > > @@ -0,0 +1,25 @@ > > +#include > > I'm not sure if all targets even provide stdint.h header, and if all targets > have uint32_t type. For most targets gcc now provides stdint.h (or uses > glibc stdint.h). > So perhaps instead of including stdint.h just do: > #ifndef __UINT32_TYPE__ > int > main () > { > return 0; > } > #else > typedef __UINT32_TYPE__ uint32_t; > > ... > #endif > ? I also added a typedef unsigned uint32_t for when sizeof(unsigned) == 4. I hope it's right. > > + > > +#define __fake_const_swab32(x) ((uint32_t)( \ > > + (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) | \ > > + (((uint32_t)(x) & (uint32_t)0x0000ff00UL) << 8) | \ > > + (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 8) | \ > > + (((uint32_t)(x) & (uint32_t)0x0000ff00UL) ) | \ > > + (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24))) > > + > > +/* Previous version of bswap optimization would detect byte swap when > none > > + happen. This test aims at catching such wrong detection to avoid > > + regressions. */ > > + > > +uint32_t > > +fake_swap32 (uint32_t in) __attribute__ ((noinline, noclone)) > > +{ > > This must not have been tested, this is a syntax error. > It should be > __attribute__ (noinline, noclone)) uint32_t > fake_swap32 (uint32_t in) > { My bad, I did catched this error but changed it only in a copy of the sources but produced the patch from git. I moved the attribute before the function name but after the return value though and it did compile and run. I used your version when redoing the patch. > ... > > (the attribute can be after the arguments only for prototypes). That's why it always sesms to be different than in my memories. I'll try to remember that (or always use prototypes in such case). > > > + return __fake_const_swab32 (in); > > +} > > + > > +int main(void) > > +{ > > + if (fake_swap32 (0x12345678) != 0x78567E12) > > + abort (); > > Use __builtin_abort (); here instead? You aren't including stdlib.h > for abort. Again, I must have copied the extern void abort() definition I've seen elsewhere but made the change only on a local copy of the sources. My apologizes. > > After fixing up the testcase, you don't need to bootstrap/regtest it again, > make check-gcc RUNTESTFLAGS=execute.exp=pr60454.c > should be enough. Right, but I made another bootstrap for another patch just after so I need to do it again. > > The rest looks good to me. Thanks. See below for the new version as well as in attachment. diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 748805e..b6d7d93 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,8 @@ +2014-03-07 Thomas Preud'homme + + PR tree-optimization/60454 + * tree-ssa-math-opts.c (find_bswap_1): Fix bswap detection. + 2014-02-23 David Holsgrove * config/microblaze/microblaze.md: Correct ashrsi_reg / lshrsi_reg names diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index f3c0c85..04ce403 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2014-03-10 Thomas Preud'homme + + PR tree-optimization/60454 + * gcc.c-torture/execute/pr60454.c: New test. + 2014-02-23 David Holsgrove * gcc/testsuite/gcc.target/microblaze/others/mem_reload.c: New test. diff --git a/gcc/testsuite/gcc.c-torture/execute/pr60454.c b/gcc/testsuite/gcc.c-torture/execute/pr60454.c new file mode 100644 index 0000000..83f2987 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c @@ -0,0 +1,31 @@ +#ifdef __UINT32_TYPE__ +typedef __UINT32_TYPE__ uint32_t; +#else +typedef unsigned uint32_t; +#endif + +#define __fake_const_swab32(x) ((uint32_t)( \ + (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) | \ + (((uint32_t)(x) & (uint32_t)0x0000ff00UL) << 8) | \ + (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 8) | \ + (((uint32_t)(x) & (uint32_t)0x0000ff00UL) ) | \ + (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24))) + +/* Previous version of bswap optimization would detect byte swap when none + happen. This test aims at catching such wrong detection to avoid + regressions. */ + +__attribute__ ((noinline, noclone)) uint32_t +fake_swap32 (uint32_t in) +{ + return __fake_const_swab32 (in); +} + +int main(void) +{ + if (sizeof (uint32_t) != 4) + return 0; + if (fake_swap32 (0x12345678) != 0x78567E12) + __builtin_abort (); + return 0; +} diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c index 8e372ed..9ff857c 100644 --- a/gcc/tree-ssa-math-opts.c +++ b/gcc/tree-ssa-math-opts.c @@ -1801,7 +1801,9 @@ find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit) if (rhs_class == GIMPLE_BINARY_RHS) { + int i; struct symbolic_number n1, n2; + unsigned HOST_WIDEST_INT mask; tree source_expr2; if (code != BIT_IOR_EXPR) @@ -1827,6 +1829,15 @@ find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit) return NULL_TREE; n->size = n1.size; + for (i = 0, mask = 0xff; i < n->size; i++, mask <<= BITS_PER_UNIT) + { + unsigned HOST_WIDEST_INT masked1, masked2; + + masked1 = n1.n & mask; + masked2 = n2.n & mask; + if (masked1 && masked2 && masked1 != masked2) + return NULL_TREE; + } n->n = n1.n | n2.n; if (!verify_symbolic_number_p (n, stmt))