From patchwork Thu Nov 26 16:45:33 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 57333 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp618946lbb; Thu, 26 Nov 2015 08:46:02 -0800 (PST) X-Received: by 10.98.16.7 with SMTP id y7mr41182074pfi.25.1448556362346; Thu, 26 Nov 2015 08:46:02 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id se8si24258637pac.136.2015.11.26.08.46.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 26 Nov 2015 08:46:02 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-415559-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-415559-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-415559-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:subject:references :in-reply-to:content-type; q=dns; s=default; b=dskQ4bGUGpuuf9Yvb SJkXMm0a2y9FWNMy/Hc9aizg05+6eEaeOjGQT+O9kbSG6E/LbC9d5LuV0QEYaP1a C+yYNAAtn+/apJwQ6YO1fPGtIsw8ZUIBtamQQwWVfKwXvVDXQGKLFSmI/J4i550q 8lsw7qRzyHTLQp2XgeblkK2GtM= 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:subject:references :in-reply-to:content-type; s=default; bh=UHZKL5XbWJPALjsXH2G7RJH ZrQE=; b=t8LdpsIS5Qn0gJHkAIMZ5AMP2tA3xQqYR8hPZq0r+0f0OeroQwi/MIQ tj9vLUjop7WgEmqH9xS3XbJ8v1cs/l3DIw6cWTtuB4gNts5A4/8ZrupLMX2IxAkW pN4jRE3J7GVwa5oHNx8HB8pqoHdX3aG3HIi4jgVCWg+6nj43SYtg= Received: (qmail 101766 invoked by alias); 26 Nov 2015 16:45:43 -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 101675 invoked by uid 89); 26 Nov 2015 16:45:43 -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, 26 Nov 2015 16:45:39 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-38-XcqvoZS3QtaiPnUd1JW-Ww-1; Thu, 26 Nov 2015 16:45:33 +0000 Received: from [10.2.206.200] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 26 Nov 2015 16:45:32 +0000 Message-ID: <5657372D.4080907@arm.com> Date: Thu, 26 Nov 2015 16:45:33 +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 Subject: Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case References: <5656E924.4030603@arm.com> <56570BDB.9070804@redhat.com> <56570EAA.1070808@arm.com> <565715D9.4070702@redhat.com> <56571895.90809@arm.com> In-Reply-To: <56571895.90809@arm.com> X-MC-Unique: XcqvoZS3QtaiPnUd1JW-Ww-1 X-IsSubscribed: yes On 26/11/15 14:35, Kyrill Tkachov wrote: > > On 26/11/15 14:23, Bernd Schmidt wrote: >> On 11/26/2015 02:52 PM, Kyrill Tkachov wrote: >>> >>> On 26/11/15 13:40, Bernd Schmidt wrote: >>>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote: >>>>> modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, >>>>> emit_b); >>>> >>>> Can this ever be true? We arrange for emit_b to set a new pseudo, >>>> don't we? Are we allowing cases where we copy a pattern that sets more >>>> than one register, and is that safe? >>> >>> You're right, this statement always sets modifieb_in_b to false. We >>> reject anything bug single_set insns >>> by this point in the code. I'll replace that with modified_in_b = false; >> >> Note that there's a mirrored test for modified_in_a, and both are already initialized to false. > > Yeah, that can be changed to just false too. I'll do that in the next revision. > Here is the updated patch. I've reindented the if-else blocks and removed the always-false initialisation of modified_a and modified_b. Re-tested on x86_64, aarch64. Ok for trunk? Thanks, Kyrill 2015-11-26 Kyrylo Tkachov PR rtl-optimization/68506 * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block first if emit_a exists or then_bb modifies 'b'. Reindent if-else blocks. 2015-11-26 Kyrylo Tkachov PR rtl-optimization/68506 * gcc.c-torture/execute/pr68506.c: New test. >> Also - careful with single_set, it can return true even for multiple sets in case there's a REG_DEAD note on one of them. You might want to strengthen your tests to also include !multiple_sets. Then, maybe instead of deleting these >> tests, turn them into gcc_checking_asserts. >> > > I see. I think the best place to do that would be in insn_valid_noce_process_p and just get it to return > false if multiple_sets (insn) is true. > > Would it be ok if I did that as a separate follow-up patch? > We don't have a testcase where this actually causes trouble and I'd like to keep the fix for > this PR as self-contained as possible. > > Thanks, > Kyrill > > > >> >> Bernd >> > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index af7a3b96f38bea429f86916fc176913cac2e6ebc..9092b377e45082ec07a30d60b070575f4dc68641 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -2206,7 +2206,6 @@ noce_try_cmove_arith (struct noce_if_info *if_info) swap insn that sets up A with the one that sets up B. If even that doesn't help, punt. */ - modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a); if (tmp_b && then_bb) { FOR_BB_INSNS (then_bb, tmp_insn) @@ -2220,40 +2219,37 @@ noce_try_cmove_arith (struct noce_if_info *if_info) } } - if (emit_a && modified_in_a) - { - modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b); - if (tmp_b && else_bb) - { - FOR_BB_INSNS (else_bb, tmp_insn) - /* Don't check inside insn_b. We will have changed it to emit_b - with a destination that doesn't conflict. */ - if (!(insn_b && tmp_insn == insn_b) - && modified_in_p (orig_a, tmp_insn)) - { - modified_in_b = true; - break; - } - - } - if (modified_in_b) - goto end_seq_and_fail; - - if (!noce_emit_bb (emit_b, else_bb, b_simple)) - goto end_seq_and_fail; + if (emit_a || modified_in_a) + { + if (tmp_b && else_bb) + { + FOR_BB_INSNS (else_bb, tmp_insn) + /* Don't check inside insn_b. We will have changed it to emit_b + with a destination that doesn't conflict. */ + if (!(insn_b && tmp_insn == insn_b) + && modified_in_p (orig_a, tmp_insn)) + { + modified_in_b = true; + break; + } + } + if (modified_in_b) + goto end_seq_and_fail; - if (!noce_emit_bb (emit_a, then_bb, a_simple)) - goto end_seq_and_fail; - } - else - { - if (!noce_emit_bb (emit_a, then_bb, a_simple)) - goto end_seq_and_fail; + if (!noce_emit_bb (emit_b, else_bb, b_simple)) + goto end_seq_and_fail; - if (!noce_emit_bb (emit_b, else_bb, b_simple)) - goto end_seq_and_fail; + if (!noce_emit_bb (emit_a, then_bb, a_simple)) + goto end_seq_and_fail; + } + else + { + if (!noce_emit_bb (emit_a, then_bb, a_simple)) + goto end_seq_and_fail; - } + if (!noce_emit_bb (emit_b, else_bb, b_simple)) + goto end_seq_and_fail; + } target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0), XEXP (if_info->cond, 1), a, b); diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c new file mode 100644 index 0000000000000000000000000000000000000000..15984edfe0812dc1dbbc8a07bc5b95997ed3acb9 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c @@ -0,0 +1,63 @@ +/* { dg-options "-fno-builtin-abort" } */ + +int a, b, m, n, o, p, s, u, i; +char c, q, y; +short d; +unsigned char e; +static int f, h; +static short g, r, v; +unsigned t; + +extern void abort (); + +int +fn1 (int p1) +{ + return a ? p1 : p1 + a; +} + +unsigned char +fn2 (unsigned char p1, int p2) +{ + return p2 >= 2 ? p1 : p1 >> p2; +} + +static short +fn3 () +{ + int w, x = 0; + for (; p < 31; p++) + { + s = fn1 (c | ((1 && c) == c)); + t = fn2 (s, x); + c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n; + v = -c; + y = 1; + for (; y; y++) + e = v == 1; + d = 0; + for (; h != 2;) + { + for (;;) + { + if (!m) + abort (); + r = 7 - f; + x = e = i | r; + q = u * g; + w = b == q; + if (w) + break; + } + break; + } + } + return x; +} + +int +main () +{ + fn3 (); + return 0; +}