From patchwork Mon Jan 11 11:10:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 565765 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id EC613140B92 for ; Mon, 11 Jan 2016 22:10:20 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=vq102LZU; dkim-atps=neutral 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=Jd+95cJpxiaLF/if+ 4VTlHPdpptu0K6fsC4HS3+GCYXHgUQsZiaLcaxxfeDmWQ0qFxb5L+CIb97ZESpzr IpjY8bHnDG85ljDPIFZ1xSpgk+Lz5RMZjxEyFrDc8ldPCbfc35P8hNhaDnsfgRAM j+vD5o2SJJTLYSezF2c8KGDSR0= 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=nKXWcDccNIFKYSKWSxMrdgr 1OrA=; b=vq102LZUrZuXlSMzCtkBLmw5lt5VHZsghYJQbeiyRSqOVvfaZLfiLnx 72LFg/77yxmdERDSenthE7KTXv8Kh7wsxZdUIoAHpLvJcvmpm9bilCEQpNkxQRaI yjkKMZWMPGcOOwDhYArXLoj+ef1yNESG4nxLpRlNDx0wHjh6P0NU= Received: (qmail 125245 invoked by alias); 11 Jan 2016 11:10:12 -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 125233 invoked by uid 89); 11 Jan 2016 11:10:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, KAM_LOTSOFHASH, RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=insns, MEM_P, fallthru, mem_p X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 11 Jan 2016 11:10:10 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EB7243A8; Mon, 11 Jan 2016 03:09:33 -0800 (PST) Received: from [10.2.206.200] (e100706-lin.cambridge.arm.com [10.2.206.200]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 265543F24D; Mon, 11 Jan 2016 03:10:08 -0800 (PST) Message-ID: <56938D8E.7040501@foss.arm.com> Date: Mon, 11 Jan 2016 11:10:06 +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/68841: Make sure one basic block doesn't clobber CC reg usage of the other References: <56740520.30908@foss.arm.com> <56740734.1080107@redhat.com> <5677C50F.2010302@foss.arm.com> <568BC82D.20004@redhat.com> <568BD193.9020802@foss.arm.com> <568BF08E.1010900@redhat.com> <568BF7FA.2060108@foss.arm.com> <568C093C.90402@foss.arm.com> <568CEE1B.6080800@foss.arm.com> <568E2E6C.4050806@foss.arm.com> <568E517F.8020201@redhat.com> <568E6BFD.6060506@foss.arm.com> <568E6C85.4010007@redhat.com> <568E6F81.7070101@foss.arm.com> <568FB59D.7020106@foss.arm.com> <568FB831.5060309@redhat.com> In-Reply-To: <568FB831.5060309@redhat.com> Hi Bernd, On 08/01/16 13:22, Bernd Schmidt wrote: > On 01/08/2016 02:11 PM, Kyrill Tkachov wrote: >> How's this? > > Hmm. Almost there, but... > >> - if (then_bb && else_bb && !a_simple && !b_simple >> - && (!bbs_ok_for_cmove_arith (then_bb, else_bb) >> - || !bbs_ok_for_cmove_arith (else_bb, then_bb))) >> + rtx orig_x = x; >> + if (then_bb && else_bb) >> + { >> + orig_x = SET_DEST (single_set (insn_a)); >> + gcc_assert (rtx_equal_p (orig_x, SET_DEST (single_set (insn_b)))); >> + } >> + >> + if (then_bb && else_bb >> + && (!bbs_ok_for_cmove_arith (then_bb, else_bb, orig_x) >> + || !bbs_ok_for_cmove_arith (else_bb, then_bb, orig_x))) >> return FALSE; > > This can be condensed to a single if statement (not repeating the then_bb && else_bb test), and orig_x can then be declared locally inside it. > > The remaining question is whether it's safe to call single_set in this way. I thought it was only guaranteed to be safe if then_simple and else_simple. I had assumed that orig_x was available in this function but I now see that it's > actually part of noce_process_if_block. I think it might be best to extend the if_info structure to also have an orig_x field. With that, I think we can even skip this particular assert (keeping the one in bbs_ok_for_cmove_arith however). > Ok, this works too. Here is a version that adds orig_x to the if_info struct and passes it down to bbs_ok_for_cmove_arith. This passes bootstrap and test on arm, aarch64, x86_64. Thanks, Kyrill 2016-01-11 Bernd Schmidt Kyrylo Tkachov PR rtl-optimization/68841 * ifcvt.c (struct noce_if_info): Add orig_x field. (bbs_ok_for_cmove_arith): Add to_rename parameter. Don't record conflicts on to_rename if it's present. Allow memory destinations in sets. (noce_try_cmove_arith): Call bbs_ok_for_cmove_arith even on simple blocks, passing orig_x to the checks. (noce_process_if_block): Set if_info->orig_x appropriately. 2016-01-11 Kyrylo Tkachov PR rtl-optimization/68841 * gcc.dg/pr68841.c: New test. * gcc.c-torture/execute/pr68841.c: New test. diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 5812ce30151ed7425780890c66e7763f5758df7e..76561d2fa03590462ea880d79377844d5d6b53f1 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -792,6 +792,9 @@ struct noce_if_info /* The SET_DEST of INSN_A. */ rtx x; + /* The original set destination that the THEN and ELSE basic blocks finally + write their result to. */ + rtx orig_x; /* True if this if block is not canonical. In the canonical form of if blocks, the THEN_BB is the block reached via the fallthru edge from TEST_BB. For the noce transformations, we allow the symmetric @@ -1896,11 +1899,13 @@ insn_valid_noce_process_p (rtx_insn *insn, rtx cc) } -/* Return true iff the registers that the insns in BB_A set do not - get used in BB_B. */ +/* Return true iff the registers that the insns in BB_A set do not get + used in BB_B. If TO_RENAME is non-NULL then it is a location that will be + renamed later by the caller and so conflicts on it should be ignored + in this function. */ static bool -bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b) +bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b, rtx to_rename) { rtx_insn *a_insn; bitmap bba_sets = BITMAP_ALLOC (®_obstack); @@ -1920,10 +1925,10 @@ bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b) BITMAP_FREE (bba_sets); return false; } - /* Record all registers that BB_A sets. */ FOR_EACH_INSN_DEF (def, a_insn) - bitmap_set_bit (bba_sets, DF_REF_REGNO (def)); + if (!(to_rename && DF_REF_REG (def) == to_rename)) + bitmap_set_bit (bba_sets, DF_REF_REGNO (def)); } rtx_insn *b_insn; @@ -1942,8 +1947,15 @@ bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b) } /* Make sure this is a REG and not some instance - of ZERO_EXTRACT or SUBREG or other dangerous stuff. */ - if (!REG_P (SET_DEST (sset_b))) + of ZERO_EXTRACT or SUBREG or other dangerous stuff. + If we have a memory destination then we have a pair of simple + basic blocks performing an operation of the form [addr] = c ? a : b. + bb_valid_for_noce_process_p will have ensured that these are + the only stores present. In that case [addr] should be the location + to be renamed. Assert that the callers set this up properly. */ + if (MEM_P (SET_DEST (sset_b))) + gcc_assert (rtx_equal_p (SET_DEST (sset_b), to_rename)); + else if (!REG_P (SET_DEST (sset_b))) { BITMAP_FREE (bba_sets); return false; @@ -2112,9 +2124,9 @@ noce_try_cmove_arith (struct noce_if_info *if_info) } } - if (then_bb && else_bb && !a_simple && !b_simple - && (!bbs_ok_for_cmove_arith (then_bb, else_bb) - || !bbs_ok_for_cmove_arith (else_bb, then_bb))) + if (then_bb && else_bb + && (!bbs_ok_for_cmove_arith (then_bb, else_bb, if_info->orig_x) + || !bbs_ok_for_cmove_arith (else_bb, then_bb, if_info->orig_x))) return FALSE; start_sequence (); @@ -3430,6 +3442,7 @@ noce_process_if_block (struct noce_if_info *if_info) /* Only operate on register destinations, and even then avoid extending the lifetime of hard registers on small register class machines. */ orig_x = x; + if_info->orig_x = orig_x; if (!REG_P (x) || (HARD_REGISTER_P (x) && targetm.small_register_classes_for_mode_p (GET_MODE (x)))) diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68841.c b/gcc/testsuite/gcc.c-torture/execute/pr68841.c new file mode 100644 index 0000000000000000000000000000000000000000..15a27e7dc382d97398ca05427f431f5ecd3b89da --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr68841.c @@ -0,0 +1,31 @@ +static inline int +foo (int *x, int y) +{ + int z = *x; + while (y > z) + z *= 2; + return z; +} + +int +main () +{ + int i; + for (i = 1; i < 17; i++) + { + int j; + int k; + j = foo (&i, 7); + if (i >= 7) + k = i; + else if (i >= 4) + k = 8 + (i - 4) * 2; + else if (i == 3) + k = 12; + else + k = 8; + if (j != k) + __builtin_abort (); + } + return 0; +} diff --git a/gcc/testsuite/gcc.dg/pr68841.c b/gcc/testsuite/gcc.dg/pr68841.c new file mode 100644 index 0000000000000000000000000000000000000000..470048cc24f0d7150ed1e3141181bc1e8472ae12 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr68841.c @@ -0,0 +1,34 @@ +/* { dg-do run } */ +/* { dg-options "-Og -fif-conversion -flive-range-shrinkage -fpeel-loops -frerun-cse-after-loop" } */ + +static inline int +foo (int *x, int y) +{ + int z = *x; + while (y > z) + z *= 2; + return z; +} + +int +main () +{ + int i; + for (i = 1; i < 17; i++) + { + int j; + int k; + j = foo (&i, 7); + if (i >= 7) + k = i; + else if (i >= 4) + k = 8 + (i - 4) * 2; + else if (i == 3) + k = 12; + else + k = 8; + if (j != k) + __builtin_abort (); + } + return 0; +}