From patchwork Thu Nov 19 10:28:36 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 56981 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp3090640lbb; Thu, 19 Nov 2015 02:29:02 -0800 (PST) X-Received: by 10.68.237.98 with SMTP id vb2mr9520256pbc.26.1447928942499; Thu, 19 Nov 2015 02:29:02 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id kw9si10992639pab.122.2015.11.19.02.29.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Nov 2015 02:29:02 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-414602-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; spf=pass (google.com: domain of gcc-patches-return-414602-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-414602-patch=linaro.org@gcc.gnu.org; dkim=pass header.i=@gcc.gnu.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=Eh52YmLV91Ba8Zu9/ PcZaJjeRHOKgmxGOswbDjesUm7IotOknGMooo2aG/1CWrVnIIBSBsG7xIm6TsuRT vrjj75cD61qc/MNKtyPelkXCg0/Y/ls/haPAflns8vd+BBwayjVVimH/mFiyt/S9 +B0FTqbYJn1Lnk9RCRnrmax/t0= 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=u19sEhudwS/0Ch3WNTU/s5r sKEY=; b=IEGdZG49HDz44UQwXw228iJwUmvsfA5aWqDVK/WzY6FFCpe66e14XYD HhrgOMInkIq1p6/47tS4uUeFYmNk5QCIrn0VZPb/X2GH5rFTAxbWWyS9qkPR5IZu ykNG0N7QSE/lcp2bq3EBg7zmPgQoFtAQ1C9kfs/9KFi+foAIkj0o= Received: (qmail 7164 invoked by alias); 19 Nov 2015 10:28:48 -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 7146 invoked by uid 89); 19 Nov 2015 10:28:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (207.82.80.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Nov 2015 10:28:46 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-36-ngMvxST1Q0y6pwwwzIRthQ-1; Thu, 19 Nov 2015 10:28:40 +0000 Received: from [10.2.206.200] ([10.1.2.79]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 19 Nov 2015 10:28:36 +0000 Message-ID: <564DA454.2080704@arm.com> Date: Thu, 19 Nov 2015 10:28:36 +0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Bernd Schmidt , GCC Patches CC: Jeff Law Subject: Re: [PATCH][RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves References: <5649E333.4090904@arm.com> <564A2339.3030308@redhat.com> <564AEE94.3070708@arm.com> <564B1934.6050300@redhat.com> <564B259A.90206@arm.com> <564BB42C.1020401@redhat.com> <564C40D1.80409@arm.com> In-Reply-To: <564C40D1.80409@arm.com> X-MC-Unique: ngMvxST1Q0y6pwwwzIRthQ-1 X-IsSubscribed: yes On 18/11/15 09:11, Kyrill Tkachov wrote: > > On 17/11/15 23:11, Bernd Schmidt wrote: >> On 11/17/2015 02:03 PM, Kyrill Tkachov wrote: >>> + || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn)))) >>> return false; >> >>> Well, I think the statement we want to make is >>> "return false from this function if the two expressions contain the same >>> register number". >> >> I looked at it again and I think I'll need more time to really figure out what's going on in this pass. >> >> However, about the above... either I'm very confused, or the actual statement here is "return false _unless_ the two expressions contain the same register number". In the testcase, the regs in question are ax and bx, which is then >> rejected if the patch has been applied. > > I'm sorry, it is my mistake in explaining. I meant to say: > "return false from this function unless the two expressions contain the same > register number" > >> >> (gdb) p tmp_reg >> (reg:SI 0 ax) >> (gdb) p cand->insn >> (insn 59 117 60 7 (set (reg:SI 4 si [orig:107 D.1813 ] [107]) >> (sign_extend:SI (reg:HI 3 bx [orig:99 D.1813 ] [99]))) >> >> And I think it would be better to strengthen that to "return false unless registers are identical". (From the above it's clear however that a plain rtx_equal_p wouldn't work since there's an extension in the operand). > > So the three relevant instructions are: > I1: (set (reg:HI 0 ax) > (mem:HI (symbol_ref:DI ("f")))) > > I2: (set (reg:SI 3 bx) > (if_then_else:SI (eq (reg:CCZ 17 flags) > (const_int 0)) > (reg:SI 0 ax) > (reg:SI 3 bx))) > > I3: (set (reg:SI 4 si) > (sign_extend:SI (reg:HI 3 bx))) > > I1 is def_insn, I3 is cand->insn. tmp_reg is 'ax'. What we want to do is reject this transformation > because the destination of def_insn (aka I1), that is 'ax', is not the operand of the extend operation > in cand->insn (aka I3). As you said, rtx_equal won't work on just SET_SRC (PATTERN (cand->insn)) because > it's an extend operation. So reg_overlap_mentioned should be appropriate. > Hope this helps. > >> >> Also, I had another look at the testcase. It uses __builtin_printf and dg-output, which is at least unusual. Try to use the normal "if (cond) abort ()". >> > > I did not try to remove the __builtin_printf because the use of 'f' there is needed to trigger this. > There are at least two other dups of this PR: 68328 and 68185 the testcases for which have an "if (cond) abort ();" form. > I'll use them instead. > For completeness' sake here's the patch I'm proposing. I've used the testcases from the other two PRs that exhibit the same problem and use the if (cond) abort (); form. Kyrill 2015-11-16 Kyrylo Tkachov PR rtl-optimization/68194 PR rtl-optimization/68328 PR rtl-optimization/68185 * ree.c (combine_reaching_defs): Reject copy_needed case if defining insn does not feed directly into candidate extension insn. 2015-11-16 Kyrylo Tkachov PR rtl-optimization/68194 * gcc.c-torture/execute/pr68185.c: Likewise. * gcc.c-torture/execute/pr68328.c: Likewise. > >> >> Bernd >> > commit 7ca1b135babbe3a542c850591e1b0e8736a19f55 Author: Kyrylo Tkachov Date: Fri Nov 13 15:01:47 2015 +0000 [RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves diff --git a/gcc/ree.c b/gcc/ree.c index b8436f2..e91d164 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -814,7 +814,30 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) rtx tmp_reg = gen_rtx_REG (GET_MODE (SET_DEST (PATTERN (cand->insn))), REGNO (SET_DEST (*dest_sub_rtx))); - if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn)))) + + /* When transforming: + (set (reg1) (expression)) + ... + (set (reg2) (any_extend (reg1))) + + into + + (set (reg2) (any_extend (expression))) + (set (reg1) (reg2)) + make sure that reg1 from the first set feeds directly into the extend. + This may not hold in a situation with an intermediate + conditional copy i.e. + I1: (set (reg3) (expression)) + I2: (set (reg1) (cond ? reg3 : reg1)) + I3: (set (reg2) (any_extend (reg1))) + + where I3 is cand, I1 is def_insn and I2 is a conditional copy. + We want to avoid transforming that into: + (set (reg2) (any_extend (expression))) + (set (reg1) (reg2)) + (set (reg1) (cond ? reg3 : reg1)). */ + if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn))) + || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn)))) return false; /* The destination register of the extension insn must not be diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68185.c b/gcc/testsuite/gcc.c-torture/execute/pr68185.c new file mode 100644 index 0000000..826531b --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr68185.c @@ -0,0 +1,29 @@ +int a, b, d = 1, e, f, o, u, w = 1, z; +short c, q, t; + +int +main () +{ + char g; + for (; d; d--) + { + while (o) + for (; e;) + { + c = b; + int h = o = z; + for (; u;) + for (; a;) + ; + } + if (t < 1) + g = w; + f = g; + g && (q = 1); + } + + if (q != 1) + __builtin_abort (); + + return 0; +} diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68328.c b/gcc/testsuite/gcc.c-torture/execute/pr68328.c new file mode 100644 index 0000000..edf244b --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr68328.c @@ -0,0 +1,44 @@ +int a, b, c = 1, d = 1, e; + +__attribute__ ((noinline, noclone)) + int foo (void) +{ + asm volatile ("":::"memory"); + return 4195552; +} + +__attribute__ ((noinline, noclone)) + void bar (int x, int y) +{ + asm volatile (""::"g" (x), "g" (y):"memory"); + if (y == 0) + __builtin_abort (); +} + +int +baz (int x) +{ + char g, h; + int i, j; + + foo (); + for (;;) + { + if (c) + h = d; + g = h < x ? h : 0; + i = (signed char) ((unsigned char) (g - 120) ^ 1); + j = i > 97; + if (a - j) + bar (0x123456, 0); + if (!b) + return e; + } +} + +int +main () +{ + baz (2); + return 0; +}