From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30780 invoked by alias); 16 Jul 2014 15:24:36 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 30743 invoked by uid 89); 16 Jul 2014 15:24:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 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; Wed, 16 Jul 2014 15:24:27 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Wed, 16 Jul 2014 16:24:23 +0100 Received: from [10.1.209.51] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 16 Jul 2014 16:24:22 +0100 Message-ID: <53C69926.4050503@arm.com> Date: Wed, 16 Jul 2014 15:27:00 -0000 From: Alan Lawrence User-Agent: Thunderbird 2.0.0.24 (X11/20101213) MIME-Version: 1.0 To: Jeff Law CC: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH] Relax check against commuting XOR and ASHIFTRT in combine.c References: <53B1B4FE.7010201@arm.com> <53B1D271.5000405@redhat.com> In-Reply-To: <53B1D271.5000405@redhat.com> X-MC-Unique: 114071616242318201 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg01157.txt.bz2 Thanks for the suggestions! I think I've got a reasonably platform-independ= ent=20 testcase that scans the rtl dump, just trying it on a few more platforms no= w... As to running on Alpha: bootstrap succeeds, and the regression testsuite do= esn't=20 raise any issues (https://gcc.gnu.org/ml/gcc-testresults/2014-07/msg01236.h= tml)=20 - and that's with a more aggressive patch that completely rolls back the=20 original r76965: Index: combine.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- combine.c (revision 212523) +++ combine.c (working copy) @@ -10218,9 +10218,6 @@ if (CONST_INT_P (XEXP (varop, 1)) /* We can't do this if we have (ashiftrt (xor)) and the constant has its sign bit set in shift_mode. */ - && !(code =3D=3D ASHIFTRT && GET_CODE (varop) =3D=3D XOR - && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)), - shift_mode)) && (new_rtx =3D simplify_const_binary_operation (code, result_mode, gen_int_mode (INTVAL (XEXP (varop, 1)), result_mode), @@ -10237,10 +10234,7 @@ logical expression, make a new logical expression, and apply the inverse distributive law. This also can't be done for some (ashiftrt (xor)). */ - if (CONST_INT_P (XEXP (varop, 1)) - && !(code =3D=3D ASHIFTRT && GET_CODE (varop) =3D=3D XOR - && 0 > trunc_int_for_mode (INTVAL (XEXP (varop, 1)), - shift_mode))) + if (CONST_INT_P (XEXP (varop, 1))) { rtx lhs =3D simplify_shift_const (NULL_RTX, code, shift_mo= de, XEXP (varop, 0), count); I'm testing this version more widely but initial indications are good. However, I've not succeeded in checking Ada on Alpha, as GCC's Ada frontend= =20 requires an Ada compiler to bootstrap. So I have to ask: does anyone actual= ly=20 use Ada on Alpha? (And if so, would they please be able to test the above p= atch?) Moreover, I don't really see we have much reason to believe the check again= st=20 commuting is required even for Ada/Alpha. GCC's internals have changed=20 substantially in the interim, with the Ada frontend no longer generating RT= L=20 directly, as we now have intervening GENERIC/GIMPLE tree stages. Unless the= re is=20 a logical/bitwise explanation for why the commuting of ashiftrc and xor is= =20 unsafe, is the best explanation that the Ada frontend was generating RTL th= at=20 may have looked OK at the time but we would now consider dubious, malformed= , bad? (E.g., these days I don't see how to produce an ashiftrt of one mode contai= ning an XOR of another without an intervening sign_extend, zero_extend or subreg= .) --Alan Jeff Law wrote: > On 06/30/14 13:05, Alan Lawrence wrote: >> combine.c includes a check which prevents >> >> (ashiftrt (xor A C2) C1) >> >> from being commuted to >> >> (xor (ashiftrt A C1) (ashiftrt C2 C1)) >> >> for constants C1, C2 if C2 has its sign bit set. >> >> Specifically, this prevents (ashiftrt (not A) C1) from being commuted to >> >> (not (ashiftrt A C1)) >> >> because the former is rewritten to (ashiftrt (xor A -1) C1) above, with >> a comment /* Make this fit the case below. */ - which it no longer does. >> >> If result_mode =3D=3D shift_mode, I can see no reason to prevent this >> normalisation (indeed, I'm not sure I can see any reason to prevent it >> even if result_mode !=3D shift_mode - but I've not managed to produce su= ch >> a case in my own testing, as there are always intervening subreg's or >> sign_extend's, or to build a toolchain on which to reproduce the >> original bug, so I'm being cautious). Hence this patch allows >> commutation if the two modes are equal. >> >> As an illustrative example, on AArch64, without this patch, compiling >> this snippet of the current arm_neon.h: >> >> typedef long long int int64x1_t __attribute__ ((__vector_size__((8)))); >> int64x1 vcgez_s64(int64x1_t a) >> { >> return (int64x1_t) {a >=3D0 ? -1 : 0}; >> } >> >> produces a sequence involving a logical not (mvn) followed by asr (plus >> some inter-bank moves), as the combiner takes (ashiftrt (not x) 63), >> "simplifies", and fails to match the resulting RTL >> >> (set (reg:DI 78 [ D.2593 ]) >> (ashiftrt:DI (xor:DI (reg:DI 0 x0 [ aD.2565 ]) >> (const_int -1 [0xffffffffffffffff])) >> (const_int 63 [0x3f]))) >> >> However, with this patch, the combiner simplifies to >> >> (set (reg:DI 84 [ D.2604 ]) >> (neg:DI (ge:DI (reg:DI 32 v0 [ aD.2569 ]) >> (const_int 0 [0])))) >> >> which matches a pattern to output the desired cmge instruction. >> >> (For the curious: the check against commutation was added in January >> 2004, r76965, https://gcc.gnu.org/ml/gcc-patches/2004-01/msg03406.html - >> however, I've not been able to build an ADA toolchain of that era, or an >> Alpha/VMS toolchain, on which to reproduce the bug; if anyone has such >> and is willing and able to investigate, by all means!) >> >> Testing: >> >> aarch64-none-elf: check-gcc (cross-tested) >> x86-none-linux-gnu: bootstrapped; check-gcc, check-ada, check-g++ >> arm-none-linux-gnueabi: bootstrapped >> arm-none-eabi: check-gcc (cross-tested) >> >> >> gcc/ChangeLog: >> >> * combine.c (simplify_shift_const_1): Allow commuting ASHIFTRT and = XOR >> if same mode. > You'll want to use your sample code from above to create a testcase.=20 > You can either dump the RTL and search it, or scan the assembly code. >=20 > Look in gcc/testsuite/gcc.target/arm for examples. >=20 > Given the original problem which prompted Kenner to make this change was= =20 > Ada related on the Alpha, you might ping rth and/or uros to see if they=20 > could do a "quick" regression of those platforms with Ada enabled. >=20 > I'm naturally hesitant to approve since this was something pretty Kenner= =20 > explicitly tried to avoid AFAICT. Kenner wasn't ever known for trying=20 > to paper over problems -- if he checked in a change like this, much more= =20 > likely than not it was well thought out and analyzed. He also probably=20 > knew combine.c better than anyone at that time (possibly even still true= =20 > today). >=20 >=20 > Jeff >=20 >=20 >=20