From patchwork Wed Nov 9 08:32:41 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 81449 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp83121qge; Wed, 9 Nov 2016 00:33:08 -0800 (PST) X-Received: by 10.98.147.93 with SMTP id b90mr31070300pfe.170.1478680388860; Wed, 09 Nov 2016 00:33:08 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id sh1si34129313pac.308.2016.11.09.00.33.08 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 09 Nov 2016 00:33:08 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-440805-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-440805-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-440805-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=Ymah9sLel3dkmdJ 7V4cOfck0gzCaj/IfpMHxs6/JtaSH1RuQdYLSRDo4/p9mzlRcf98f2t5Ei7qSDiw QyzmNszVV+9ngbXOwJOTXf5XBOw4IQJYviTE2nTfoa4bQFRQrQOIIbIqlZR8xj4m AXsAj39lDOMXirseL67BAJee0uf4= 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 :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; s=default; bh=jtzmWvbxRDgvjn9DzvrYx vTsdlc=; b=Lv5OjlALHn/jTYk3mTd42bj/ASl4dECuPzHsS8BIMHxPWVZ8l5aYK 9jMUwYLn4kIJ0CA5fZrRhXdnKVgu0InKaDgVS/jEg10ieLmhOb6pfADUZ6d9rFQX X361j/jhGLImq/I3Qcd7UOCik5IvtBg8foUGfG1YSz5dVh06pPB5bM= Received: (qmail 73978 invoked by alias); 9 Nov 2016 08:32:54 -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 73963 invoked by uid 89); 9 Nov 2016 08:32:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy=2 X-HELO: mail-it0-f41.google.com Received: from mail-it0-f41.google.com (HELO mail-it0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 09 Nov 2016 08:32:43 +0000 Received: by mail-it0-f41.google.com with SMTP id u205so259072228itc.0 for ; Wed, 09 Nov 2016 00:32:43 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=YF/FfyBTnqMPWwlrUlTk0vHfJOXIOmdmxejj0eU79yo=; b=YgMHovVRCYKh34ABX7uOX4wJnCLzWNOQS44EL87kC9xM3HMTOR8cgxZRD5xDij5XAY j0SDUi4tG9ZpNPbSbZ/P16l/Do1SeelTTWSHE03rIFliCgKk+2DYfsY9bxnQ/8qCdiio lWKUoUcf/QFu44huACiaq7sYyl0djOhuZgeb0g3xrTnANcxn5xirrji57O1lVKcsDt+t DgePHWh122f6tLW4qnrvTS0UeEYIvxvTCY6H4x+i8b09i9ZQXp9zdFZ8rkz6wOpEVv8x HIBgq/HMQg81r68YbhTYIPB9xegjuB4HQ5hGOYEzR9ocaNJ92PYgQtvuGynYL/OoopEx EoNw== X-Gm-Message-State: ABUngvdAl3pYBsbThnWrcmdJpB7ipwYMEzlkK07UpWtIKeNS9QZ7T12G5SyDVKnQVHiGSucMlKx8iSRDtJJ0qSn0 X-Received: by 10.36.43.193 with SMTP id h184mr11075571ita.29.1478680361932; Wed, 09 Nov 2016 00:32:41 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.12.169 with HTTP; Wed, 9 Nov 2016 00:32:41 -0800 (PST) In-Reply-To: References: From: Prathamesh Kulkarni Date: Wed, 9 Nov 2016 14:02:41 +0530 Message-ID: Subject: Re: [match.pd] Fix for PR35691 To: Richard Biener Cc: gcc Patches , Christophe Lyon , Kyrill Tkachov X-IsSubscribed: yes On 8 November 2016 at 16:46, Richard Biener wrote: > On Tue, 8 Nov 2016, Prathamesh Kulkarni wrote: > >> On 8 November 2016 at 13:23, Richard Biener wrote: >> > On Mon, 7 Nov 2016, Prathamesh Kulkarni wrote: >> > >> >> On 7 November 2016 at 23:06, Prathamesh Kulkarni >> >> wrote: >> >> > On 7 November 2016 at 15:43, Richard Biener wrote: >> >> >> On Fri, 4 Nov 2016, Prathamesh Kulkarni wrote: >> >> >> >> >> >>> On 4 November 2016 at 13:41, Richard Biener wrote: >> >> >>> > On Thu, 3 Nov 2016, Marc Glisse wrote: >> >> >>> > >> >> >>> >> On Thu, 3 Nov 2016, Richard Biener wrote: >> >> >>> >> >> >> >>> >> > > > > The transform would also work for vectors (element_precision for >> >> >>> >> > > > > the test but also a value-matching zero which should ensure the >> >> >>> >> > > > > same number of elements). >> >> >>> >> > > > Um sorry, I didn't get how to check vectors to be of equal length by a >> >> >>> >> > > > matching zero. >> >> >>> >> > > > Could you please elaborate on that ? >> >> >>> >> > > >> >> >>> >> > > He may have meant something like: >> >> >>> >> > > >> >> >>> >> > > (op (cmp @0 integer_zerop@2) (cmp @1 @2)) >> >> >>> >> > >> >> >>> >> > I meant with one being @@2 to allow signed vs. Unsigned @0/@1 which was the >> >> >>> >> > point of the pattern. >> >> >>> >> >> >> >>> >> Oups, that's what I had written first, and then I somehow managed to confuse >> >> >>> >> myself enough to remove it so as to remove the call to types_match :-( >> >> >>> >> >> >> >>> >> > > So the last operand is checked with operand_equal_p instead of >> >> >>> >> > > integer_zerop. But the fact that we could compute bit_ior on the >> >> >>> >> > > comparison results should already imply that the number of elements is the >> >> >>> >> > > same. >> >> >>> >> > >> >> >>> >> > Though for equality compares we also allow scalar results IIRC. >> >> >>> >> >> >> >>> >> Oh, right, I keep forgetting that :-( And I have no idea how to generate one >> >> >>> >> for a testcase, at least until the GIMPLE FE lands... >> >> >>> >> >> >> >>> >> > > On platforms that have IOR on floats (at least x86 with SSE, maybe some >> >> >>> >> > > vector mode on s390?), it would be cool to do the same for floats (most >> >> >>> >> > > likely at the RTL level). >> >> >>> >> > >> >> >>> >> > On GIMPLE view-converts could come to the rescue here as well. Or we cab >> >> >>> >> > just allow bit-and/or on floats as much as we allow them on pointers. >> >> >>> >> >> >> >>> >> Would that generate sensible code on targets that do not have logic insns for >> >> >>> >> floats? Actually, even on x86_64 that generates inefficient code, so there >> >> >>> >> would be some work (for instance grep finds no gen_iordf3, only gen_iorv2df3). >> >> >>> >> >> >> >>> >> I am also a bit wary of doing those obfuscating optimizations too early... >> >> >>> >> a==0 is something that other optimizations might use. long >> >> >>> >> c=(long&)a|(long&)b; (double&)c==0; less so... >> >> >>> >> >> >> >>> >> (and I am assuming that signaling NaNs don't make the whole transformation >> >> >>> >> impossible, which might be wrong) >> >> >>> > >> >> >>> > Yeah. I also think it's not so much important - I just wanted to mention >> >> >>> > vectors... >> >> >>> > >> >> >>> > Btw, I still think we need a more sensible infrastructure for passes >> >> >>> > to gather, analyze and modify complex conditions. (I'm always pointing >> >> >>> > to tree-affine.c as an, albeit not very good, example for handling >> >> >>> > a similar problem) >> >> >>> Thanks for mentioning the value-matching capture @@, I wasn't aware of >> >> >>> this match.pd feature. >> >> >>> The current patch keeps it restricted to only bitwise operators on integers. >> >> >>> Bootstrap+test running on x86_64-unknown-linux-gnu. >> >> >>> OK to commit if passes ? >> >> >> >> >> >> +/* PR35691: Transform >> >> >> + (x == 0 & y == 0) -> (x | typeof(x)(y)) == 0. >> >> >> + (x != 0 | y != 0) -> (x | typeof(x)(y)) != 0. */ >> >> >> + >> >> >> >> >> >> Please omit the vertical space >> >> >> >> >> >> +(for bitop (bit_and bit_ior) >> >> >> + cmp (eq ne) >> >> >> + (simplify >> >> >> + (bitop (cmp @0 integer_zerop) (cmp @1 integer_zerop)) >> >> >> >> >> >> if you capture the first integer_zerop as @2 then you can re-use it... >> >> >> >> >> >> + (if (INTEGRAL_TYPE_P (TREE_TYPE (@0)) >> >> >> + && INTEGRAL_TYPE_P (TREE_TYPE (@1)) >> >> >> + && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE >> >> >> (@1))) >> >> >> + (cmp (bit_ior @0 (convert @1)) { build_zero_cst (TREE_TYPE (@0)); >> >> >> >> >> >> ... here inplace of the { build_zero_cst ... }. >> >> >> >> >> >> Ok with that changes. >> >> > Thanks, committed the attached version as r241915. >> >> ugh, the svn commit message has: >> >> >> >> testsuite/ >> >> * gcc.dg/pr35691-1.c: New test-case. >> >> * gcc.dg/pr35691-4.c: Likewise. >> >> >> >> pr35691-4.c was a typo, should be pr35691-2.c :/ >> >> However testsuite/ChangeLog correctly has entry for pr35691-2.c >> >> Is it possible to edit the commit message for r241915 ? >> >> Sorry about this. >> > >> > No, just leave it as-is. >> Hi, >> Chritstophe reported to me that the commit caused test-cases >> pr35691-1.c and pr35691-2.c (which were added by the commit) >> to FAIL for cortex-a5: >> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241915/arm-none-linux-gnueabihf/diff-gcc-rh60-arm-none-linux-gnueabihf-arm-cortex-a5-vfpv3-d16-fp16.txt >> >> It seems truth_andif_expr is not simplified to bit_and_expr on >> cortex-a5 as for x86_64 (and other arm variants). >> The differences in dumps start from 004t.gimple for pr35691-1.c: >> >> x86_64 gimple dump: >> foo (int z0, unsigned int z1) >> { >> int D.1800; >> int t0; >> int t1; >> int t2; >> >> _1 = z0 == 0; >> t0 = (int) _1; >> _2 = z1 == 0; >> t1 = (int) _2; >> _3 = t0 != 0; >> _4 = t1 != 0; >> _5 = _3 & _4; >> t2 = (int) _5; >> D.1800 = t2; >> return D.1800; >> } >> >> cortex-a5 gimple dump: >> foo (int z0, unsigned int z1) >> { >> int iftmp.0; >> int D.4176; >> int t0; >> int t1; >> int t2; >> >> _1 = z0 == 0; >> t0 = (int) _1; >> _2 = z1 == 0; >> t1 = (int) _2; >> if (t0 != 0) goto ; else goto ; >> : >> if (t1 != 0) goto ; else goto ; >> : >> iftmp.0 = 1; >> goto ; >> : >> iftmp.0 = 0; >> : >> t2 = iftmp.0; >> D.4176 = t2; >> return D.4176; >> } >> >> Since the pattern expects truth_andif_expr to be converted to bit_and_expr, >> it fails to match for cortex-a5. >> This seems to happen only for cortex-a5 (the other variants a9, a15, >> a57 are OK). >> >> Is my assumption that truth_andif_expr would be always converted to bit_and_expr >> for above case incorrect ? > > Yes, it depends on LOGICAL_OP_SHORT_CIRCUIT. Thanks, I wasn't aware of that. I adjusted the test-case in the attached patch, to always contain bit_and_expr/bit_ior_expr, which passes on cortex-a5 and powerpc. I will update PR35691 with the comment that it's partially fixed, for targets when LOGICAL_OP_NON_SHORT_CIRCUIT is true. Cross-testing on arm*-*-* in progress. Ok to commit if passes ? Thanks, Prathamesh > > Richard. > >> Thanks, >> Prathamesh >> > >> > Richard. >> > >> >> Regards, >> >> Prathamesh >> >> > >> >> >> >> >> >> Richard. >> >> >> >> >> > >> > -- >> > Richard Biener >> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) >> >> > > -- > Richard Biener > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) diff --git a/gcc/testsuite/gcc.dg/pr35691-1.c b/gcc/testsuite/gcc.dg/pr35691-1.c index 5211f815..125923d 100644 --- a/gcc/testsuite/gcc.dg/pr35691-1.c +++ b/gcc/testsuite/gcc.dg/pr35691-1.c @@ -5,7 +5,7 @@ int foo(int z0, unsigned z1) { int t0 = (z0 == 0); int t1 = (z1 == 0); - int t2 = (t0 && t1); + int t2 = (t0 & t1); return t2; } diff --git a/gcc/testsuite/gcc.dg/pr35691-2.c b/gcc/testsuite/gcc.dg/pr35691-2.c index 90cbf6d..70f68a6 100644 --- a/gcc/testsuite/gcc.dg/pr35691-2.c +++ b/gcc/testsuite/gcc.dg/pr35691-2.c @@ -5,7 +5,7 @@ int foo(int z0, unsigned z1) { int t0 = (z0 != 0); int t1 = (z1 != 0); - int t2 = (t0 || t1); + int t2 = (t0 | t1); return t2; }