From patchwork Fri May 6 09:38:35 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chung-Lin Tang X-Patchwork-Id: 94350 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]) by ozlabs.org (Postfix) with SMTP id 47B64B6F4F for ; Fri, 6 May 2011 19:39:06 +1000 (EST) Received: (qmail 24647 invoked by alias); 6 May 2011 09:39:04 -0000 Received: (qmail 24595 invoked by uid 22791); 6 May 2011 09:39:00 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 06 May 2011 09:38:41 +0000 Received: (qmail 29975 invoked from network); 6 May 2011 09:38:39 -0000 Received: from unknown (HELO ?60.245.67.143?) (cltang@127.0.0.2) by mail.codesourcery.com with ESMTPA; 6 May 2011 09:38:39 -0000 Message-ID: <4DC3C19B.8060409@codesourcery.com> Date: Fri, 06 May 2011 17:38:35 +0800 From: Chung-Lin Tang User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.17) Gecko/20110414 Thunderbird/3.1.10 MIME-Version: 1.0 To: Jeff Law CC: gcc-patches Subject: Re: [PATCH] Canonicalize compares in combine [2/3] Modifications to try_combine() References: <4DB19D03.8050606@codesourcery.com> <4DB5B883.3000602@redhat.com> <4DB6B001.9050404@codesourcery.com> <4DBAE0F2.2010506@redhat.com> In-Reply-To: <4DBAE0F2.2010506@redhat.com> X-IsSubscribed: yes 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 Hi Jeff, I have verified the patch with a native bootstrap + testsuite run on powerpc-linux (32-bit), results were clean. Attached is a single patch with the 1+2 combine parts together, with comments updated. Please check if they feel descriptive enough. I haven't updated the CANONICALIZE_COMPARISON stuff, as we discussed it doesn't look like absolutely needed right now. As for the const0_rtx compare, because the entire case is guarded by a CONST_INT_P, I think it should be safe. Is this now okay for trunk? Thanks, Chung-Lin On 2011/4/30 12:01 AM, Jeff Law wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 04/26/11 05:44, Chung-Lin Tang wrote: >> >> Hi Jeff, thanks for reviewing this quite convoluted patch :) > FWIW, I don't think it's necessarily your patch that is convoluted, but > instead the original code. It's also the case that I haven't spent > nearly as much time in the combiner as I have in other parts of GCC. > > >> Yes I'm not doing anything weird here, although the definition of the >> CANONICALIZE_COMPARISON macro does not seem to forbid (other ports) from >> doing so, as long as the comparison result is equivalent. It is >> currently called only at the end of simplify_comparison(), which itself >> possibly modifies the compare operands. >> >> Considering not a lot of targets currently use CANONICALIZE_COMPARISON, >> maybe it would be feasible to slightly change its interface? Maybe >> adding a bool parameter to indicate whether the individual operands have >> to be retained equivalent. > That can certainly be done if necessary. I'm just not sure it's needed, > at least not at this time. If you want to make that change, feel free > to submit it. > >> >>> >>> I've generally found that avoiding const0_rtx is wise. Instead I >>> suggest using CONST0_RTX (mode) to get the constant in the proper mode. >>> >> >> Do you mean the (op1 == const0_rtx) test? > Yes. It's a fairly minor issue. > > >> >>> Given the tangled mess this code happens to be, could you please do a >>> bootstrap test on target with and without SELECT_CC_MODE. x86 would be >>> a great example of the former, powerpc the latter (use the build farm if >>> you need to). For ppc, just bootstrapping would be fine; I don't think >>> a full regression test is needed, just some verification that we don't >>> break ppc build should be sufficient. >>> >> >> Bootstrap and test on x86 is completed already, both 32 and 64 bit. >> Bootstrap on ARM itself is also completed, using a Pandaboard. I'll try >> doing a ppc build too. > Thanks. Given that x86 & ARM are both SELECT_CC_MODE targets, I suspect > they're OK. I mostly want a sniff test on a target that doesn't define > SELECT_CC_MODE (ppc for example). > > Thanks, > jeff > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ > > iQEcBAEBAgAGBQJNuuDyAAoJEBRtltQi2kC7gMQH/3hANrSFa3MSCTUvNxaiv9sy > HLoVYgcVXxNCNwYm/uSxZ3titiqyqB7ySrYPEezXCqdaNJeUSk+5v9w23wszyoel > bITxU6FC1P9wgHCRVNc9NsxpBJeCZleDSPHcrqAdbW8yO6J59qtaaRAlsBjgz11f > Isg1X+apG0Cy6DZ8knNRDVv9+HyJNLTYCg+90sSQv2Of1obp0b34sq/C2e03+oHz > gVBkgCvkLazcvYxLrdyCgXfyXvNMbMfSeVclJzpikJZAOAJBCwceYd16wg9ohBZR > y1g31oZ4teYcLz6c+yWEiWmZpVbeLnZSB9/bkvwP9lrQwkmKyxP33kdYHl/VP1E= > =ZZwe > -----END PGP SIGNATURE----- Index: combine.c =================================================================== --- combine.c (revision 173473) +++ combine.c (working copy) @@ -450,6 +450,7 @@ int); static int recog_for_combine (rtx *, rtx, rtx *); static rtx gen_lowpart_for_combine (enum machine_mode, rtx); +static enum rtx_code simplify_compare_const (enum rtx_code, rtx, rtx *); static enum rtx_code simplify_comparison (enum rtx_code, rtx *, rtx *); static void update_table_tick (rtx); static void record_value_for_reg (rtx, rtx, rtx); @@ -3046,58 +3047,98 @@ if (i1 == 0 && added_sets_2 && GET_CODE (PATTERN (i3)) == SET && GET_CODE (SET_SRC (PATTERN (i3))) == COMPARE - && XEXP (SET_SRC (PATTERN (i3)), 1) == const0_rtx + && CONST_INT_P (XEXP (SET_SRC (PATTERN (i3)), 1)) && rtx_equal_p (XEXP (SET_SRC (PATTERN (i3)), 0), i2dest)) { -#ifdef SELECT_CC_MODE - rtx *cc_use; - enum machine_mode compare_mode; -#endif + rtx newpat_dest; + rtx *cc_use_loc = NULL, cc_use_insn = NULL_RTX; + rtx op0 = i2src, op1 = XEXP (SET_SRC (PATTERN (i3)), 1); + enum machine_mode compare_mode, orig_compare_mode; + enum rtx_code compare_code = UNKNOWN, orig_compare_code = UNKNOWN; newpat = PATTERN (i3); - SUBST (XEXP (SET_SRC (newpat), 0), i2src); + newpat_dest = SET_DEST (newpat); + compare_mode = orig_compare_mode = GET_MODE (newpat_dest); - i2_is_used = 1; - -#ifdef SELECT_CC_MODE - /* See if a COMPARE with the operand we substituted in should be done - with the mode that is currently being used. If not, do the same - processing we do in `subst' for a SET; namely, if the destination - is used only once, try to replace it with a register of the proper - mode and also replace the COMPARE. */ if (undobuf.other_insn == 0 - && (cc_use = find_single_use (SET_DEST (newpat), i3, - &undobuf.other_insn)) - && ((compare_mode = SELECT_CC_MODE (GET_CODE (*cc_use), - i2src, const0_rtx)) - != GET_MODE (SET_DEST (newpat)))) + && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, + &cc_use_insn))) { - if (can_change_dest_mode (SET_DEST (newpat), added_sets_2, - compare_mode)) + compare_code = orig_compare_code = GET_CODE (*cc_use_loc); + compare_code = simplify_compare_const (compare_code, + op0, &op1); +#ifdef CANONICALIZE_COMPARISON + CANONICALIZE_COMPARISON (compare_code, op0, op1); +#endif + } + + /* Do the rest only if op1 is const0_rtx, which may be the + result of simplification. */ + if (op1 == const0_rtx) + { + /* If a single use of the CC is found, prepare to modify it + when SELECT_CC_MODE returns a new CC-class mode, or when + the above simplify_compare_const() returned a new comparison + operator. undobuf.other_insn is assigned the CC use insn + when modifying it. */ + if (cc_use_loc) { - unsigned int regno = REGNO (SET_DEST (newpat)); - rtx new_dest; - - if (regno < FIRST_PSEUDO_REGISTER) - new_dest = gen_rtx_REG (compare_mode, regno); - else +#ifdef SELECT_CC_MODE + enum machine_mode new_mode + = SELECT_CC_MODE (compare_code, op0, op1); + if (new_mode != orig_compare_mode + && can_change_dest_mode (SET_DEST (newpat), + added_sets_2, new_mode)) { - SUBST_MODE (regno_reg_rtx[regno], compare_mode); - new_dest = regno_reg_rtx[regno]; + unsigned int regno = REGNO (newpat_dest); + compare_mode = new_mode; + if (regno < FIRST_PSEUDO_REGISTER) + newpat_dest = gen_rtx_REG (compare_mode, regno); + else + { + SUBST_MODE (regno_reg_rtx[regno], compare_mode); + newpat_dest = regno_reg_rtx[regno]; + } } +#endif + /* Cases for modifying the CC-using comparison. */ + if (compare_code != orig_compare_code + /* ??? Do we need to verify the zero rtx? */ + && XEXP (*cc_use_loc, 1) == const0_rtx) + { + /* Replace cc_use_loc with entire new RTX. */ + SUBST (*cc_use_loc, + gen_rtx_fmt_ee (compare_code, compare_mode, + newpat_dest, const0_rtx)); + undobuf.other_insn = cc_use_insn; + } + else if (compare_mode != orig_compare_mode) + { + /* Just replace the CC reg with a new mode. */ + SUBST (XEXP (*cc_use_loc, 0), newpat_dest); + undobuf.other_insn = cc_use_insn; + } + } - SUBST (SET_DEST (newpat), new_dest); - SUBST (XEXP (*cc_use, 0), new_dest); - SUBST (SET_SRC (newpat), - gen_rtx_COMPARE (compare_mode, i2src, const0_rtx)); - } - else - undobuf.other_insn = 0; + /* Now we modify the current newpat: + First, SET_DEST(newpat) is updated if the CC mode has been + altered. For targets without SELECT_CC_MODE, this should be + optimized away. */ + if (compare_mode != orig_compare_mode) + SUBST (SET_DEST (newpat), newpat_dest); + /* This is always done to propagate i2src into newpat. */ + SUBST (SET_SRC (newpat), + gen_rtx_COMPARE (compare_mode, op0, op1)); + /* Create new version of i2pat if needed; the below PARALLEL + creation needs this to work correctly. */ + if (! rtx_equal_p (i2src, op0)) + i2pat = gen_rtx_SET (VOIDmode, i2dest, op0); + i2_is_used = 1; } -#endif } - else #endif + + if (i2_is_used == 0) { /* It is possible that the source of I2 or I1 may be performing an unneeded operation, such as a ZERO_EXTEND of something @@ -10811,6 +10852,191 @@ return gen_rtx_CLOBBER (omode, const0_rtx); } +/* Try to simplify a comparison between OP0 and a constant OP1, + where CODE is the comparison code that will be tested, into a + (CODE OP0 const0_rtx) form. + + The result is a possibly different comparison code to use. + *POP1 may be updated. */ + +static enum rtx_code +simplify_compare_const (enum rtx_code code, rtx op0, rtx *pop1) +{ + enum machine_mode mode = GET_MODE (op0); + unsigned int mode_width = GET_MODE_BITSIZE (mode); + HOST_WIDE_INT const_op = INTVAL (*pop1); + + /* Get the constant we are comparing against and turn off all bits + not on in our mode. */ + if (mode != VOIDmode) + const_op = trunc_int_for_mode (const_op, mode); + + /* If we are comparing against a constant power of two and the value + being compared can only have that single bit nonzero (e.g., it was + `and'ed with that bit), we can replace this with a comparison + with zero. */ + if (const_op + && (code == EQ || code == NE || code == GE || code == GEU + || code == LT || code == LTU) + && mode_width <= HOST_BITS_PER_WIDE_INT + && exact_log2 (const_op) >= 0 + && nonzero_bits (op0, mode) == (unsigned HOST_WIDE_INT) const_op) + { + code = (code == EQ || code == GE || code == GEU ? NE : EQ); + const_op = 0; + } + + /* Similarly, if we are comparing a value known to be either -1 or + 0 with -1, change it to the opposite comparison against zero. */ + if (const_op == -1 + && (code == EQ || code == NE || code == GT || code == LE + || code == GEU || code == LTU) + && num_sign_bit_copies (op0, mode) == mode_width) + { + code = (code == EQ || code == LE || code == GEU ? NE : EQ); + const_op = 0; + } + + /* Do some canonicalizations based on the comparison code. We prefer + comparisons against zero and then prefer equality comparisons. + If we can reduce the size of a constant, we will do that too. */ + switch (code) + { + case LT: + /* < C is equivalent to <= (C - 1) */ + if (const_op > 0) + { + const_op -= 1; + code = LE; + /* ... fall through to LE case below. */ + } + else + break; + + case LE: + /* <= C is equivalent to < (C + 1); we do this for C < 0 */ + if (const_op < 0) + { + const_op += 1; + code = LT; + } + + /* If we are doing a <= 0 comparison on a value known to have + a zero sign bit, we can replace this with == 0. */ + else if (const_op == 0 + && mode_width <= HOST_BITS_PER_WIDE_INT + && (nonzero_bits (op0, mode) + & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1))) + == 0) + code = EQ; + break; + + case GE: + /* >= C is equivalent to > (C - 1). */ + if (const_op > 0) + { + const_op -= 1; + code = GT; + /* ... fall through to GT below. */ + } + else + break; + + case GT: + /* > C is equivalent to >= (C + 1); we do this for C < 0. */ + if (const_op < 0) + { + const_op += 1; + code = GE; + } + + /* If we are doing a > 0 comparison on a value known to have + a zero sign bit, we can replace this with != 0. */ + else if (const_op == 0 + && mode_width <= HOST_BITS_PER_WIDE_INT + && (nonzero_bits (op0, mode) + & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1))) + == 0) + code = NE; + break; + + case LTU: + /* < C is equivalent to <= (C - 1). */ + if (const_op > 0) + { + const_op -= 1; + code = LEU; + /* ... fall through ... */ + } + /* (unsigned) < 0x80000000 is equivalent to >= 0. */ + else if (mode_width <= HOST_BITS_PER_WIDE_INT + && (unsigned HOST_WIDE_INT) const_op + == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) + { + const_op = 0; + code = GE; + break; + } + else + break; + + case LEU: + /* unsigned <= 0 is equivalent to == 0 */ + if (const_op == 0) + code = EQ; + /* (unsigned) <= 0x7fffffff is equivalent to >= 0. */ + else if (mode_width <= HOST_BITS_PER_WIDE_INT + && (unsigned HOST_WIDE_INT) const_op + == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1) + { + const_op = 0; + code = GE; + } + break; + + case GEU: + /* >= C is equivalent to > (C - 1). */ + if (const_op > 1) + { + const_op -= 1; + code = GTU; + /* ... fall through ... */ + } + + /* (unsigned) >= 0x80000000 is equivalent to < 0. */ + else if (mode_width <= HOST_BITS_PER_WIDE_INT + && (unsigned HOST_WIDE_INT) const_op + == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) + { + const_op = 0; + code = LT; + break; + } + else + break; + + case GTU: + /* unsigned > 0 is equivalent to != 0 */ + if (const_op == 0) + code = NE; + /* (unsigned) > 0x7fffffff is equivalent to < 0. */ + else if (mode_width <= HOST_BITS_PER_WIDE_INT + && (unsigned HOST_WIDE_INT) const_op + == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1) + { + const_op = 0; + code = LT; + } + break; + + default: + break; + } + + *pop1 = GEN_INT (const_op); + return code; +} + /* Simplify a comparison between *POP0 and *POP1 where CODE is the comparison code that will be tested. @@ -11000,186 +11226,11 @@ && (GET_CODE (op0) == COMPARE || COMPARISON_P (op0)))) break; - /* Get the constant we are comparing against and turn off all bits - not on in our mode. */ + /* Try to simplify the compare to constant, possibly changing the + comparison op, and/or changing op1 to zero. */ + code = simplify_compare_const (code, op0, &op1); const_op = INTVAL (op1); - if (mode != VOIDmode) - const_op = trunc_int_for_mode (const_op, mode); - op1 = GEN_INT (const_op); - /* If we are comparing against a constant power of two and the value - being compared can only have that single bit nonzero (e.g., it was - `and'ed with that bit), we can replace this with a comparison - with zero. */ - if (const_op - && (code == EQ || code == NE || code == GE || code == GEU - || code == LT || code == LTU) - && mode_width <= HOST_BITS_PER_WIDE_INT - && exact_log2 (const_op) >= 0 - && nonzero_bits (op0, mode) == (unsigned HOST_WIDE_INT) const_op) - { - code = (code == EQ || code == GE || code == GEU ? NE : EQ); - op1 = const0_rtx, const_op = 0; - } - - /* Similarly, if we are comparing a value known to be either -1 or - 0 with -1, change it to the opposite comparison against zero. */ - - if (const_op == -1 - && (code == EQ || code == NE || code == GT || code == LE - || code == GEU || code == LTU) - && num_sign_bit_copies (op0, mode) == mode_width) - { - code = (code == EQ || code == LE || code == GEU ? NE : EQ); - op1 = const0_rtx, const_op = 0; - } - - /* Do some canonicalizations based on the comparison code. We prefer - comparisons against zero and then prefer equality comparisons. - If we can reduce the size of a constant, we will do that too. */ - - switch (code) - { - case LT: - /* < C is equivalent to <= (C - 1) */ - if (const_op > 0) - { - const_op -= 1; - op1 = GEN_INT (const_op); - code = LE; - /* ... fall through to LE case below. */ - } - else - break; - - case LE: - /* <= C is equivalent to < (C + 1); we do this for C < 0 */ - if (const_op < 0) - { - const_op += 1; - op1 = GEN_INT (const_op); - code = LT; - } - - /* If we are doing a <= 0 comparison on a value known to have - a zero sign bit, we can replace this with == 0. */ - else if (const_op == 0 - && mode_width <= HOST_BITS_PER_WIDE_INT - && (nonzero_bits (op0, mode) - & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1))) - == 0) - code = EQ; - break; - - case GE: - /* >= C is equivalent to > (C - 1). */ - if (const_op > 0) - { - const_op -= 1; - op1 = GEN_INT (const_op); - code = GT; - /* ... fall through to GT below. */ - } - else - break; - - case GT: - /* > C is equivalent to >= (C + 1); we do this for C < 0. */ - if (const_op < 0) - { - const_op += 1; - op1 = GEN_INT (const_op); - code = GE; - } - - /* If we are doing a > 0 comparison on a value known to have - a zero sign bit, we can replace this with != 0. */ - else if (const_op == 0 - && mode_width <= HOST_BITS_PER_WIDE_INT - && (nonzero_bits (op0, mode) - & ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1))) - == 0) - code = NE; - break; - - case LTU: - /* < C is equivalent to <= (C - 1). */ - if (const_op > 0) - { - const_op -= 1; - op1 = GEN_INT (const_op); - code = LEU; - /* ... fall through ... */ - } - - /* (unsigned) < 0x80000000 is equivalent to >= 0. */ - else if (mode_width <= HOST_BITS_PER_WIDE_INT - && (unsigned HOST_WIDE_INT) const_op - == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - { - const_op = 0, op1 = const0_rtx; - code = GE; - break; - } - else - break; - - case LEU: - /* unsigned <= 0 is equivalent to == 0 */ - if (const_op == 0) - code = EQ; - - /* (unsigned) <= 0x7fffffff is equivalent to >= 0. */ - else if (mode_width <= HOST_BITS_PER_WIDE_INT - && (unsigned HOST_WIDE_INT) const_op - == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1) - { - const_op = 0, op1 = const0_rtx; - code = GE; - } - break; - - case GEU: - /* >= C is equivalent to > (C - 1). */ - if (const_op > 1) - { - const_op -= 1; - op1 = GEN_INT (const_op); - code = GTU; - /* ... fall through ... */ - } - - /* (unsigned) >= 0x80000000 is equivalent to < 0. */ - else if (mode_width <= HOST_BITS_PER_WIDE_INT - && (unsigned HOST_WIDE_INT) const_op - == (unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - { - const_op = 0, op1 = const0_rtx; - code = LT; - break; - } - else - break; - - case GTU: - /* unsigned > 0 is equivalent to != 0 */ - if (const_op == 0) - code = NE; - - /* (unsigned) > 0x7fffffff is equivalent to < 0. */ - else if (mode_width <= HOST_BITS_PER_WIDE_INT - && (unsigned HOST_WIDE_INT) const_op - == ((unsigned HOST_WIDE_INT) 1 << (mode_width - 1)) - 1) - { - const_op = 0, op1 = const0_rtx; - code = LT; - } - break; - - default: - break; - } - /* Compute some predicates to simplify code below. */ equality_comparison_p = (code == EQ || code == NE);