From patchwork Thu Dec 3 09:33:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 57625 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp3386345lbb; Thu, 3 Dec 2015 01:33:57 -0800 (PST) X-Received: by 10.66.243.3 with SMTP id wu3mr11649631pac.135.1449135237400; Thu, 03 Dec 2015 01:33:57 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id 14si10950062pfm.135.2015.12.03.01.33.57 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 03 Dec 2015 01:33:57 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-416225-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-416225-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-416225-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:content-type; q= dns; s=default; b=kFnRxUWd6cR7JCykVLQZ2GAaP3Hp/WgiTj6p7KAYcNLqtL rOUo0ixI3zoKebJF+BCNHw6hk5/bBB2ESUqaRUbmfmZu1zwKE7nBi8wHdndXYKZ1 zXT7HFGPm77Uxy5fY4vwImxWToPuHhiGyGUt+GmYcuE9Hh0DGHM6oEbYzPtWU= 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:content-type; s= default; bh=z/Z/rbCMlP+laMsmJhrahZTC/FY=; b=un5I/s2ZwTAJh3HbpX65 hJe1bnukt7UYEV5AAilPgPEe2Sqcn72ME04qsmWQ1UgtXuQyXzpbIVVKdTXOM1Yh cgAt0/fcLpC8qWJspLEUy80k9xoIwge0JRid8qkio3ejsHb/BWvFnH/z4qyVuP4O O+INIllUPYD7jEF07diB6Lk= Received: (qmail 118447 invoked by alias); 3 Dec 2015 09:33: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 118433 invoked by uid 89); 3 Dec 2015 09:33:42 -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; Thu, 03 Dec 2015 09:33:41 +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-27-5E5fd0HJSOCpprafbJ5mqA-1; Thu, 03 Dec 2015 09:33:35 +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, 3 Dec 2015 09:33:35 +0000 Message-ID: <56600C6F.1010701@arm.com> Date: Thu, 03 Dec 2015 09:33:35 +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: GCC Patches Subject: [PATCH][RTL-ifcvt] PR rtl-optimization/68624: Clean up logic that checks for clobbering conflicts across basic blocks X-MC-Unique: 5E5fd0HJSOCpprafbJ5mqA-1 X-IsSubscribed: yes Hi all, In this fix I want to simplify the control flow of the code that chooses the order in which to emit the then and else basic blocks (and their associated emit_a and emit_b instructions). Currently we check the then block and only if there is a modification there we check the else block and make a decision there. IMO it's much simpler if we check both blocks and write the logic that chooses the order as a simple IF-ELSEIF-ELSE block that only emits the blocks and doesn't try to do any other checks. The bug in the logic that was preventing the clobber check from being performed in this PR was in the code: 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) where the second if condition should have been: if (tmp_a && else_bb) Just changing the tmp_b to tmp_a in that condition would have fixed the wrong-code part of this PR as we would have ended up rejecting if-conversion. However, there is a valid if-conversion opportunity here, we just have to emit emit_a followed by else_bb, which the current control flow made awkward, which is why I'm suggesting this small rewrite. Bootstrapped and tested on x86_64, aarch64, arm. Ok for trunk? Thanks, Kyrill 2015-12-03 Kyrylo Tkachov PR rtl-optimization/68624 * ifcvt.c (noce_try_cmove_arith): Check clobbers of temp regs in both blocks if they exist and simplify the logic choosing the order to emit them in. 2015-12-03 Kyrylo Tkachov PR rtl-optimization/68624 * gcc.c-torture/execute/pr68624.c: New test. diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 86b6ef7246ceddd223e93922737496af3d93f148..ef23c4cda66e6a659eee9b30089a6cc056cea30f 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -2202,10 +2202,6 @@ noce_try_cmove_arith (struct noce_if_info *if_info) } } - /* If insn to set up A clobbers any registers B depends on, try to - 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) { @@ -2220,31 +2216,33 @@ 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_a && else_bb) { - 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)) { - 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; - } + modified_in_b = true; + break; } - if (modified_in_b) - goto end_seq_and_fail; + } + /* If insn to set up A clobbers any registers B depends on, try to + swap insn that sets up A with the one that sets up B. If even + that doesn't help, punt. */ + if (modified_in_a && !modified_in_b) + { 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 + else if (!modified_in_a) { if (!noce_emit_bb (emit_a, then_bb, a_simple)) goto end_seq_and_fail; @@ -2252,6 +2250,8 @@ noce_try_cmove_arith (struct noce_if_info *if_info) if (!noce_emit_bb (emit_b, else_bb, b_simple)) goto end_seq_and_fail; } + else + 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/pr68624.c b/gcc/testsuite/gcc.c-torture/execute/pr68624.c new file mode 100644 index 0000000000000000000000000000000000000000..abb716b1550038cb3d0e96e8917b7ed0ba8bfa83 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr68624.c @@ -0,0 +1,30 @@ +int b, c, d, e = 1, f, g, h, j; + +static int +fn1 () +{ + int a = c; + if (h) + return 9; + g = (c || b) % e; + if ((g || f) && b) + return 9; + e = d; + for (c = 0; c > -4; c--) + ; + if (d) + c--; + j = c; + return d; +} + +int +main () +{ + fn1 (); + + if (c != -4) + __builtin_abort (); + + return 0; +}