From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19965 invoked by alias); 8 Jan 2016 13:12:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 19948 invoked by uid 89); 8 Jan 2016 13:12:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.0 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=kyrylo, Kyrylo, gen_reg_rtx, sk:noce_tr 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; Fri, 08 Jan 2016 13:12:01 +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 99E0A49; Fri, 8 Jan 2016 05:11:25 -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 99B083F246; Fri, 8 Jan 2016 05:11:58 -0800 (PST) Message-ID: <568FB59D.7020106@foss.arm.com> Date: Fri, 08 Jan 2016 13:12:00 -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> In-Reply-To: <568E6F81.7070101@foss.arm.com> Content-Type: multipart/mixed; boundary="------------020204010903090202070703" X-SW-Source: 2016-01/txt/msg00409.txt.bz2 This is a multi-part message in MIME format. --------------020204010903090202070703 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2447 On 07/01/16 14:00, Kyrill Tkachov wrote: > > On 07/01/16 13:47, Bernd Schmidt wrote: >> On 01/07/2016 02:45 PM, Kyrill Tkachov wrote: >>> >>> On 07/01/16 11:52, Bernd Schmidt wrote: >>>> I looked and this is indeed the case. Still slightly scary. Should the >>>> MEM be rtx_equal_p to TO_RENAME? It would be good to test or at least >>>> assert this. >>>> >>> >>> I tried asserting that and it caused trouble because a bit further up in >>> noce_process_if_block it does: >>> /* Only operate on register destinations, and even then avoid extending >>> the lifetime of hard registers on small register class machines. */ >>> orig_x = x; >>> if (!REG_P (x) >>> || (HARD_REGISTER_P (x) >>> && targetm.small_register_classes_for_mode_p (GET_MODE (x)))) >>> { >>> if (GET_MODE (x) == BLKmode) >>> return FALSE; >>> >>> if (GET_CODE (x) == ZERO_EXTRACT >>> && (!CONST_INT_P (XEXP (x, 1)) >>> || !CONST_INT_P (XEXP (x, 2)))) >>> return FALSE; >>> >>> x = gen_reg_rtx (GET_MODE (GET_CODE (x) == STRICT_LOW_PART >>> ? XEXP (x, 0) : x)); >>> } >>> >>> It changes X to a register and after noce_try_cmove_arith it emits the >>> store. >> >> This suggests that orig_x needs to be passed to bbs_ok_for_cmove_arith, even disregarding the question of using it to assert that only expected MEMs are being modified. >> > > Yes, which should be the original destinations of insn_a and insn_b of if_info. > I'll work on that. Though if we execute the above path then x will be a fresh new pseudo and so > will not cause any conflicts anyway. > Here is the latest version. I passed down the original set destination to bbs_ok_for_cmove_arith and asserted that it's the only memory store if a store occurs. Bootstrapped and tested on arm, aarch64, x86_64. How's this? Thanks, Kyrill 2016-01-08 Bernd Schmidt Kyrylo Tkachov PR rtl-optimization/68841 * ifcvt.c (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. 2016-01-08 Kyrylo Tkachov PR rtl-optimization/68841 * gcc.dg/pr68841.c: New test. * gcc.c-torture/execute/pr68841.c: New test. --------------020204010903090202070703 Content-Type: text/x-patch; name="ifcvt-tmp.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="ifcvt-tmp.patch" Content-length: 4193 diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 5812ce30151ed7425780890c66e7763f5758df7e..50fe69d13294ca1ddbac17cccecf436f7f9d231a 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -1896,11 +1896,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 +1922,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 +1944,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 +2121,16 @@ 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))) + 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; start_sequence (); 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; +} --------------020204010903090202070703--