From patchwork Mon Nov 23 15:12:32 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 57163 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp1493292lbb; Mon, 23 Nov 2015 07:12:57 -0800 (PST) X-Received: by 10.98.65.135 with SMTP id g7mr16653585pfd.141.1448291576969; Mon, 23 Nov 2015 07:12:56 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id up8si19933812pac.111.2015.11.23.07.12.56 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 Nov 2015 07:12:56 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-415024-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-415024-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-415024-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=s1jUMkaSYkdqrB77W I3pbtyupeooKvH/xYcAJYJX8pf8hdu/sAP7XiDpLKl1arlMotKtRu8XbjRGn+JHt VycOi1mtQsnKRIK8JM9MR58lMg7/KNrcUmeVtJ/yyjgKHSK7sNxcgRWFnQOjqi6n Pktx+3G35FLwOPWgDyuAFefhyA= 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=vVAUIW3NISphFRz8yaLSRgg qlR8=; b=q4XMFjCfgJ+l1VHdLfoguxjKF6Qa2wgi4gSjcOrFDvzDfToHmOpaHFe LROWxOBmb/NQSpVz4FIZ7OmKjHKollJkSMX8PErR84vO+bAPJebTsMqle2g9i7Uj aAyHbH/NWvTkn7fBnTIyY+1nKApj92mnbFTrDDlGp+hVLrXN4s/k= Received: (qmail 47692 invoked by alias); 23 Nov 2015 15:12:41 -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 47680 invoked by uid 89); 23 Nov 2015 15:12:41 -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) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 23 Nov 2015 15:12:39 +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-11-8yXttbnKSd6CE3ZqEqJDUg-1; Mon, 23 Nov 2015 15:12:33 +0000 Received: from [10.2.206.200] ([10.1.2.79]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 23 Nov 2015 15:12:32 +0000 Message-ID: <56532CE0.9050802@arm.com> Date: Mon, 23 Nov 2015 15:12:32 +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> <564DA454.2080704@arm.com> <564E7A47.1090404@redhat.com> In-Reply-To: <564E7A47.1090404@redhat.com> X-MC-Unique: 8yXttbnKSd6CE3ZqEqJDUg-1 X-IsSubscribed: yes Hi Bernd, On 20/11/15 01:41, Bernd Schmidt wrote: >>> 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. > > Yeah, so just use the src_reg variable for the comparison. I still don't see why you wouldn't want to use the stronger test. But the whole thing still feels not completely ideal somehow, so after reading through ree.c for a while and > getting a better feeling for how it works, I think the following (which you said is equivalent) would be the most understandable and direct fix. > > You said that the two tests should be equivalent, and I agree. I've not found cases where the change makes a difference, other than the testcase. Would you mind running this version through the testsuite and committing if it passes? > > I've shrunk the comment; massive explanations like this for every bug are inappropriate IMO, and the example also duplicates an earlier comment in the same function. And, as I said earlier, the way you placed the comment is confusing > because only one part of the following if statement is related to it. > Thanks for the comments, here is the final patch that I'll be committing. It passed testing on arm, aarch64, x86_64. Thanks, Kyrill 2015-11-23 Bernd Schmidt Kyrylo Tkachov PR rtl-optimization/68194 PR rtl-optimization/68328 PR rtl-optimization/68185 * ree.c (combine_reaching_defs): Reject copy_needed case if copies_list is not empty. 2015-11-23 Kyrylo Tkachov PR rtl-optimization/68194 * gcc.c-torture/execute/pr68185.c: Likewise. * gcc.c-torture/execute/pr68328.c: Likewise. > > Bernd commit ceecbb45212e2c2a6650000fabba03e07f6fcbe4 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..9d94843 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -770,6 +770,12 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) if (state->defs_list.length () != 1) return false; + /* We don't have the structure described above if there are + conditional moves in between the def and the candidate, + and we will not handle them correctly. See PR68194. */ + if (state->copies_list.length () > 0) + return false; + /* We require the candidate not already be modified. It may, for example have been changed from a (sign_extend (reg)) into (zero_extend (sign_extend (reg))). 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; +}