From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2493 invoked by alias); 24 Oct 2014 17:04:33 -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 2473 invoked by uid 89); 24 Oct 2014 17:04:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,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; Fri, 24 Oct 2014 17:04:30 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Fri, 24 Oct 2014 18:04:27 +0100 Received: from [10.1.209.51] ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Fri, 24 Oct 2014 18:04:25 +0100 Message-ID: <544A8698.5060309@arm.com> Date: Fri, 24 Oct 2014 17:11:00 -0000 From: Alan Lawrence User-Agent: Thunderbird 2.0.0.24 (X11/20101213) MIME-Version: 1.0 To: Rainer Orth 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> <53C69926.4050503@arm.com> <53C80023.6000100@arm.com> <5409FBB1.3040509@redhat.com> <541AA89C.9070005@arm.com> <87ppe70wld.fsf@igel.home> <5449326C.9040301@arm.com> <544A3D6E.4000408@arm.com> In-Reply-To: X-MC-Unique: 114102418042702901 Content-Type: multipart/mixed; boundary="------------070001010009030205050306" X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg02584.txt.bz2 This is a multi-part message in MIME format. --------------070001010009030205050306 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: quoted-printable Content-length: 5827 Well, I'd be happy with that. Curiously, that patch is identical to what I = have=20 here...and that's not what I got having built gcc with=20 --target=3Dsparc-sun-solaris2.11, but on further investigation it looks lik= e the=20 require-effective-target-ilp32/64 is not working correctly on my setup. FWIW I've also tried MIPS (both 32- and 64-bit tests appear to work), IA64= =20 (64-bit test fine, not sure if ilp32 is supported), microblaze (32-bit), an= d=20 AArch64 with -mabi=3Dilp32. Hence, I attach an alternative/possible patch w= hich=20 adds these platforms too. However, since we have talked about removing the target selector altogether= : the=20 only arch I've found so far which has failed, was Alpha, and widening the r= egex=20 to match "le" as well as "ge" then passes on Alpha too. To be honest I expe= ct=20 that if we were to go that route, we would find a deluge of smaller platfor= ms on=20 which the test fails; but if as testsuite maintainer you think that's=20 appropriate - certainly I'd be willing to try that i.e. to exclude them as = they=20 turn up. A second alternative patch also attached. (FWIW I'll be away and u= nable=20 to commit anything before Monday.) More generally: really the test wants to be a unit test on combine_simplify= _rtx,=20 independent of front-end, expand, platform-specific insns, etc., but since = we=20 can't do that - whilst adding more platforms generally seems good, it is ma= ybe=20 not essential, and may increase fragility. (In answer to your point about adding an effective-target in target-support= s.exp=20 - yes, could do that, but it's difficult to come up with a good characteriz= ation=20 of what the criteria is, and I don't see it'd generalize to any other tests= at=20 all :(....) --Alan Rainer Orth wrote: > Alan Lawrence writes: >=20 >> Rainer Orth wrote: >>>> However, as a quick first step, does adding the ilp32 / lp64 (and keep= ing >>>> the architectures list for now) solve the immediate problem? Patch >>>> attached, OK for trunk? >>> No, as I said this is wrong for biarch targets like sparc and i386. >> When you say no this does not solve the immediate problem, are you saying >> that you are (still) seeing test failures with the require-effective-tar= get >> patch applied? Or is the issue that this would not execute the tests as >=20 > I didn't try that patch yet, but the target part is wrong, as I tried to > explain. Consider the sparc case:=20 >=20 > * if you configure for sparc-sun-solaris2.11, you default to -m32 > (i.e. ilp32), while -m64 is lp64 >=20 > * if you configure for sparcv9-sun-solaris2.11 instead, you default to > -m64 (lp64), but get ilp32 with -m32 >=20 > So, irrespective of the sparc vs. sparc64 (which is wrong, btw., the > canonical form for 64-bit-default sparc is sparcv9) forms, you can get > ilp32 and lp64 with both. >=20 > Similar issues hold for i?86 vs. x86_64 and probably other biarch > targets like powerpc vs. powerpc64, so you need to use the most generic > forms of the target names in you target lists. >=20 >> widely as might be possible? In principle I'm quite happy to relax the >> target patterns, although have been having issues with sparc (below)... >> >> Re. "what the architectures have in common" is largely that these are the >> primary/secondary archs on which I've checked the test passes! I can now >> add mips and microblaze to this list, however I'm nervous of dropping the >> target entirely given the very large number of target architectures gcc >> supports; and e.g. IA64 (in ILP32 mode) generates an ashiftrt:DI by 31 >> places, not ashiftrt:SI, which does not match the simplification criteria >> in combine.c. >=20 > As I stated before, such target lists without any explanation are bound > to confuse future readers/testers: at the very least, add comments > explaining what those lists have in common. OTOH, at this stage it > might be best to just drop the target list for now, learn which targets > pass and fail the tests, and then reintroduce them or, better yet, add > an effective-target keyword which matches them. Otherwise, you'll never > get test coverage beyond your current list. >=20 >>> This should be something like=20 >>> >>> { target aarch64*-*-* i?86-*-* powerpc*-*-* sparc*-*-* x86_64-*-* } >>> >>> E.g. sparc-sun-solaris2.11 with -m64 is lp64, but would be excluded by >>> your target list. Keep the list sorted alphabetically and best add an >>> explanation so others know what those targets have in common. >> So I've built a stage-1 compiler with --target=3Dsparc-sun-solaris2.11, = and I find >> >> * without -m64, my "dg-require-effective-target ilp32" causes the 32-b= it >> test to execute, and pass; "dg-require-effective-target lp64" prevents >> execution of the 64-bit test (which would fail) - so all as expected and >> desired. >> >> * with -lp64, behaviour is as previous (this is probably expected) >=20 > Huh? What's -lp64? >=20 >> * with -m64, "dg-require-effective-target ilp32" still causes the test= to >> execute (but it fails, as the RTL now has an ashiftrt:DI by 31 places, >> which doesn't meet the simplification criteria in combine.c - this is >> pretty much as expected). "dg-require-effective-target lp64" stops the >> 64-bit test from executing however (despite that it would now pass). >> >> Can you clarify what I should be doing on sparc, therefore? >=20 > It's not only about sparc, but about all biarch targets. The following > patch (which only includes the parts strictly necessary to avoid the > failures, nothing else I suggested above) works for me on > sparc-sun-solaris2.11 (-m32 and -m64), x86_64-unknown-linux-gnu (-m64 > and -m32), and i686-unknown-linux-gnu (-m32 and -m64): the first test is > run for 64-bit only, while the second one only for 32-bit: --------------070001010009030205050306 Content-Type: text/x-patch; name=ashiftrt_all_platforms.patch Content-Transfer-Encoding: quoted-printable Content-Disposition: inline; filename="ashiftrt_all_platforms.patch" Content-length: 1536 diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.= dg/combine_ashiftrt_1.c index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f94303= 0b77583bb7570 100644 --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c @@ -1,4 +1,7 @@ -/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*= -*-*} } */ +/* Target architectures which have been found to produce the expected RTL + (neg:DI (ge:DI ...)) when compiling for ILP32. */ +/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerp= c*-*-* sparc*-*-* x86_64-*-*} } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options "-O2 -fdump-rtl-combine-all" } */ =20 typedef long long int int64_t; diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.= dg/combine_ashiftrt_2.c index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c= 15d6b6c74930d 100644 --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c @@ -1,4 +1,7 @@ -/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */ +/* Target architectures where RTL has been found to produce the expected + (neg:SI (ge:SI ...)) when compiling for ILP32. */ +/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* m= ips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */ +/* { dg-require-effective-target ilp32} */ /* { dg-options "-O2 -fdump-rtl-combine-all" } */ =20 typedef long int32_t;= --------------070001010009030205050306 Content-Type: text/x-patch; name=ashiftrt_more_platforms.patch Content-Transfer-Encoding: quoted-printable Content-Disposition: inline; filename="ashiftrt_more_platforms.patch" Content-length: 1536 diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c b/gcc/testsuite/gcc.= dg/combine_ashiftrt_1.c index 90e64fd10dc358f10ad03a90041605bc3ccb7011..6f92c253ce95ce16bdfd7f94303= 0b77583bb7570 100644 --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_1.c @@ -1,4 +1,7 @@ -/* { dg-do compile {target sparc64*-*-* aarch64*-*-* x86_64-*-* powerpc64*= -*-*} } */ +/* Target architectures which have been found to produce the expected RTL + (neg:DI (ge:DI ...)) when compiling for ILP32. */ +/* { dg-do compile {target aarch64*-*-* ia64-*-* i?86-*-* mips*-*-* powerp= c*-*-* sparc*-*-* x86_64-*-*} } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options "-O2 -fdump-rtl-combine-all" } */ =20 typedef long long int int64_t; diff --git a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c b/gcc/testsuite/gcc.= dg/combine_ashiftrt_2.c index fd6827caed230ea5dd2d6ec4431b11bf826531ea..c921d62b70681ca1cd0c51ab17c= 15d6b6c74930d 100644 --- a/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c +++ b/gcc/testsuite/gcc.dg/combine_ashiftrt_2.c @@ -1,4 +1,7 @@ -/* { dg-do compile {target arm*-*-* i?86-*-* powerpc-*-* sparc-*-*} } */ +/* Target architectures where RTL has been found to produce the expected + (neg:SI (ge:SI ...)) when compiling for ILP32. */ +/* { dg-do compile {target aarch64*-*-* arm*-*-* i?86-*-* microblaze-*-* m= ips*-*-* powerpc*-*-* sparc*-*-* x86_64-*-*} } */ +/* { dg-require-effective-target ilp32} */ /* { dg-options "-O2 -fdump-rtl-combine-all" } */ =20 typedef long int32_t;= --------------070001010009030205050306--