From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23328 invoked by alias); 28 May 2011 17:58:55 -0000 Received: (qmail 23316 invoked by uid 22791); 28 May 2011 17:58:53 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-ww0-f51.google.com (HELO mail-ww0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 28 May 2011 17:58:38 +0000 Received: by wwf26 with SMTP id 26so2566919wwf.8 for ; Sat, 28 May 2011 10:58:37 -0700 (PDT) Received: by 10.227.196.82 with SMTP id ef18mr3312986wbb.114.1306605517317; Sat, 28 May 2011 10:58:37 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id fm14sm1971060wbb.7.2011.05.28.10.58.35 (version=TLSv1/SSLv3 cipher=OTHER); Sat, 28 May 2011 10:58:36 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Subject: PR 49145: Another (zero_extend (const_int ...)) in combine Date: Sun, 29 May 2011 09:30:00 -0000 Message-ID: <877h9ahflh.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2011-05/txt/msg02252.txt.bz2 Three places in combine.c use SUBST to perform recursive replacements on operands. Two of them (subst and known_cond) handle the special case of a zero_extend operand being optimised to a constant. This patch deals with the third, make_compound_operation. In the PR, make_compound_operation is able to use nonzero_bits to optimise a zero_extend argument to a constant. The new rtx is then (zero_extend:M (const_int N)), which is invalid. Rather than add a copy of the known_cond code to make_compound_operation, I decided to split the subst handling of this case out into a separate function that could be used in all three places. I've included the subreg handling as well as the zero_extend handling, even though make_compound_operation already handles that case correctly. However, I've cowardly not removed the known_cond subreg handling: else if (code == SUBREG) { enum machine_mode inner_mode = GET_MODE (SUBREG_REG (x)); rtx new_rtx, r = known_cond (SUBREG_REG (x), cond, reg, val); if (SUBREG_REG (x) != r) { /* We must simplify subreg here, before we lose track of the original inner_mode. */ new_rtx = simplify_subreg (GET_MODE (x), r, inner_mode, SUBREG_BYTE (x)); if (new_rtx) return new_rtx; else SUBST (SUBREG_REG (x), r); } return x; } The new function would also handle the case described in the comment. However, I was afraid that we might rely on simplify_subreg being used for all simplified operands here, while also relying on it only being used for constants in subst. (This comes from a general fear that combine is special in the way that it handles compound operations. Calling the generic simplification routines too often could cause us to turn combine's preferred representation into the representation normally used elsewhere.) However, I'm happy to change it so that either the new function calls simplify_subreg for everything, or that known_cond only calls it for constants. Let me know if you'd prefer one or the other. (TBH, I'd prefer the latter for consistency with the unary case.) The known_cond code has the comment: /* We don't have to handle SIGN_EXTEND here, because even in the case of replacing something with a modeless CONST_INT, a CONST_INT is already (supposed to be) a valid sign extension for its narrower mode, which implies it's already properly sign-extended for the wider mode. Now, for ZERO_EXTEND, the story is different. */ While that is true, I don't see any point in going out of our way _not_ to handle sign_extend in the same way. Or indeed all unary operators. Testing UNARY_P seems justified on the basis that simplify_unary_operation requires the mode of the inner operand, and that losing that mode without giving simplify_unary_operation a chance could therefore lead to wrong results. I'd rather not see us hard-code the knowledge of which operators actually care. I think it's justified for subst_operand not to handle binary operators in the same way. If we did want these functions to simplify binary operators in some cases, I think that code is more naturally done as an extension to: /* If this is a commutative operation, the changes to the operands may have made it noncanonical. */ if (COMMUTATIVE_ARITH_P (x) && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1))) { tem = XEXP (x, 0); SUBST (XEXP (x, 0), XEXP (x, 1)); SUBST (XEXP (x, 1), tem); } Binary and unary operations are different because the mode of the inner operand is implied by the outer mode. Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested on mips-linux-gnu. OK to install? Richard gcc/ PR rtl-optimization/49145 * combine.c (subst_operand): New function, split out from... (subst): ...here and generalise ZERO_EXTEND handlig to all unary operations. (make_compound_operation): Call subst_operand. (known_cond): Likewise. Do not use separate ZERO_EXTEND check. gcc/testsuite/ PR rtl-optimization/49145 From Ryan Mansfield * gcc.c-torture/compile/pr49145.c: New test. Index: gcc/combine.c =================================================================== --- gcc/combine.c 2011-05-28 18:25:40.000000000 +0100 +++ gcc/combine.c 2011-05-28 18:27:31.000000000 +0100 @@ -4973,6 +4973,46 @@ find_split_point (rtx *loc, rtx insn, bo } } +/* Replace operand I of *LOC with NEW_RTX. + + In some cases, substituting a constant into an existing rtx can + lose vital information, such as the inner mode of a SUBREG or a + ZERO_EXTEND operation. Return true if this substitution falls into + that category, and replace *LOC with a simplified form of the result. + Return false if *LOC points to the same rtx as before. */ + +static bool +subst_operand (rtx *loc, int i, rtx new_rtx) +{ + rtx x, tem; + + x = *loc; + if (CONST_INT_P (new_rtx) || GET_CODE (new_rtx) == CONST_DOUBLE) + { + if (GET_CODE (x) == SUBREG) + { + tem = simplify_subreg (GET_MODE (x), new_rtx, + GET_MODE (SUBREG_REG (x)), + SUBREG_BYTE (x)); + if (!tem) + tem = gen_rtx_CLOBBER (GET_MODE (x), const0_rtx); + } + else if (UNARY_P (x)) + tem = simplify_unary_operation (GET_CODE (x), GET_MODE (x), + new_rtx, GET_MODE (XEXP (x, 0))); + else + tem = NULL_RTX; + + if (tem) + { + *loc = tem; + return true; + } + } + SUBST (XEXP (x, i), new_rtx); + return false; +} + /* Throughout X, replace FROM with TO, and return the result. The result is TO if X is FROM; otherwise the result is X, but its contents may have been modified. @@ -5210,27 +5250,8 @@ #define COMBINE_RTX_EQUAL_P(X,Y) \ if (GET_CODE (new_rtx) == CLOBBER && XEXP (new_rtx, 0) == const0_rtx) return new_rtx; - if (GET_CODE (x) == SUBREG - && (CONST_INT_P (new_rtx) - || GET_CODE (new_rtx) == CONST_DOUBLE)) - { - enum machine_mode mode = GET_MODE (x); - - x = simplify_subreg (GET_MODE (x), new_rtx, - GET_MODE (SUBREG_REG (x)), - SUBREG_BYTE (x)); - if (! x) - x = gen_rtx_CLOBBER (mode, const0_rtx); - } - else if (CONST_INT_P (new_rtx) - && GET_CODE (x) == ZERO_EXTEND) - { - x = simplify_unary_operation (ZERO_EXTEND, GET_MODE (x), - new_rtx, GET_MODE (XEXP (x, 0))); - gcc_assert (x); - } - else - SUBST (XEXP (x, i), new_rtx); + if (subst_operand (&x, i, new_rtx)) + break; } } } @@ -7887,7 +7908,8 @@ make_compound_operation (rtx x, enum rtx if (fmt[i] == 'e') { new_rtx = make_compound_operation (XEXP (x, i), next_code); - SUBST (XEXP (x, i), new_rtx); + if (subst_operand (&x, i, new_rtx)) + break; } else if (fmt[i] == 'E') for (j = 0; j < XVECLEN (x, i); j++) @@ -8942,37 +8964,15 @@ known_cond (rtx x, enum rtx_code cond, r return x; } - /* We don't have to handle SIGN_EXTEND here, because even in the - case of replacing something with a modeless CONST_INT, a - CONST_INT is already (supposed to be) a valid sign extension for - its narrower mode, which implies it's already properly - sign-extended for the wider mode. Now, for ZERO_EXTEND, the - story is different. */ - else if (code == ZERO_EXTEND) - { - enum machine_mode inner_mode = GET_MODE (XEXP (x, 0)); - rtx new_rtx, r = known_cond (XEXP (x, 0), cond, reg, val); - - if (XEXP (x, 0) != r) - { - /* We must simplify the zero_extend here, before we lose - track of the original inner_mode. */ - new_rtx = simplify_unary_operation (ZERO_EXTEND, GET_MODE (x), - r, inner_mode); - if (new_rtx) - return new_rtx; - else - SUBST (XEXP (x, 0), r); - } - - return x; - } fmt = GET_RTX_FORMAT (code); for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--) { if (fmt[i] == 'e') - SUBST (XEXP (x, i), known_cond (XEXP (x, i), cond, reg, val)); + { + if (subst_operand (&x, i, known_cond (XEXP (x, i), cond, reg, val))) + break; + } else if (fmt[i] == 'E') for (j = XVECLEN (x, i) - 1; j >= 0; j--) SUBST (XVECEXP (x, i, j), known_cond (XVECEXP (x, i, j), Index: gcc/testsuite/gcc.c-torture/compile/pr49145.c =================================================================== --- /dev/null 2011-05-28 09:03:52.070295797 +0100 +++ gcc/testsuite/gcc.c-torture/compile/pr49145.c 2011-05-28 18:27:31.000000000 +0100 @@ -0,0 +1,30 @@ +static int +func1 (int a, int b) +{ + return b ? a : a / b; +} + +static unsigned char +func2 (unsigned char a, int b) +{ + return b ? a : b; +} + +int i; + +void +func3 (const int arg) +{ + for (i = 0; i != 10; i = foo ()) + { + if (!arg) + { + int j; + for (j = 0; j < 5; j += 1) + { + int *ptr; + *ptr = func2 (func1 (arg, *ptr), foo (arg)); + } + } + } +}