From patchwork Thu Mar 22 20:27:45 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Stump X-Patchwork-Id: 148332 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 A5BA4B6F62 for ; Fri, 23 Mar 2012 07:28:20 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1333052900; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Subject:Mime-Version:Content-Type:From:In-Reply-To:Date:Cc: Message-Id:References:To:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=iGBIu6xp1M/rqVejV8/nWC/mD7I=; b=i70aoOPeRZXH2Tq v0JOktaXFfFBHWibT/t8QPhcMNQ6gXePHjHTPANOemtwpI2RYyNptBvBq4WNuj1d n6mOYrAhLs6wi688aTWb2oNQ7LyT+NZkJPlcliueDxjWV56bpIFXwepLKQSZLRVb ApY9JeJKx1p8S1kl9eiDw9l4xC/k= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Subject:Mime-Version:Content-Type:From:In-Reply-To:Date:Cc:Message-Id:References:To:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=Gy2k8wQCuhPHPR2oObPJaZTtbVyno7F9FabFqP4bMv5G8cZ1vkUQSeUWop3fQf 7WMhyBo1jbwdPqKr/e5dzgnvOCjSE8oPvsmVmfw/lGZT6u4qmOuANtKxsT3c66Yo X90LO8aOrxJyU7Kij6q8wyUP8lVxHwxzwmbdPHNBmhtU4=; Received: (qmail 11789 invoked by alias); 22 Mar 2012 20:28:13 -0000 Received: (qmail 11743 invoked by uid 22791); 22 Mar 2012 20:28:05 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, TW_SV, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from qmta10.emeryville.ca.mail.comcast.net (HELO qmta10.emeryville.ca.mail.comcast.net) (76.96.30.17) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Mar 2012 20:27:48 +0000 Received: from omta16.emeryville.ca.mail.comcast.net ([76.96.30.72]) by qmta10.emeryville.ca.mail.comcast.net with comcast id ok3a1i0011ZMdJ4AAkToGm; Thu, 22 Mar 2012 20:27:48 +0000 Received: from up.mrs.kithrup.com ([24.4.193.8]) by omta16.emeryville.ca.mail.comcast.net with comcast id okTm1i00F0BKwT48ckTn9s; Thu, 22 Mar 2012 20:27:47 +0000 Subject: Re: remove wrong code in immed_double_const Mime-Version: 1.0 (Apple Message framework v1084) From: Mike Stump In-Reply-To: Date: Thu, 22 Mar 2012 13:27:45 -0700 Cc: gcc-patches Patches , Richard Guenther Message-Id: <83448720-93D1-483D-8B8C-08B672399E1B@comcast.net> References: <5FF5A724-3FE1-4E97-8124-542A0B8259FE@comcast.net> <87obrvd6fh.fsf@talisman.home> <87haxmgqoo.fsf@talisman.home> <7C6A7462-C1D3-4765-83FF-3B3C726D92E5@comcast.net> <8762e09sgc.fsf@talisman.home> <0A5CBD0C-FC94-4637-B230-1A83372DE91A@comcast.net> To: Richard Sandiford 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 On Mar 22, 2012, at 3:16 AM, Richard Sandiford wrote: > Sorry, meant we should leave the svn version as it is. Ah, I see now, I agree, I removed that bit (extend in the floating point case) of my change from the patch. > I think this should be s/is smaller than/is different from/. Sounds good, fixed. > Should be: > > if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false)) > /* Sorry, we have no way to represent overflows this > wide. To fix, add constant support wider than > CONST_DOUBLE. */ > gcc_assert (GET_MODE_BITSIZE (mode) > 2 * HOST_BITS_PER_WIDE_INT) As you noted, you do mean: if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false)) /* Sorry, we have no way to represent overflows this wide. To fix, add constant support wider than CONST_DOUBLE. */ gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT) Fixed. > Nitlet, but line is wider than 80 chars. Probably easiest fix is: > > tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c); > tem = force_const_mem (GET_MODE (x), tem); Fixed. >> + if (shift < 2*HOST_BITS_PER_WIDE_INT-1 >> + || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT) > > Missing spaces around binary operators. Fixed all instances of 2*HOST and INT-1, there were many, not just this one. Some pre-date my patch. >> @@ -1219,15 +1220,10 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode, >> else >> lv = CONST_DOUBLE_LOW (op), hv = CONST_DOUBLE_HIGH (op); >> >> - if (op_mode == VOIDmode) >> - { >> - /* We don't know how to interpret negative-looking numbers in >> - this case, so don't try to fold those. */ >> - if (hv < 0) >> - return 0; >> - } >> - else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2) >> - ; >> + if (op_mode == VOIDmode >> + || GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2) >> + /* We should never get a negative number. */ >> + gcc_assert (hv >= 0); >> else >> hv = 0, lv &= GET_MODE_MASK (op_mode); >> > > Sorry, with this bit, I meant that the current svn code is correct > for GET_MODE_BITSIZE (op_mode) == HOST_BITS_PER_WIDE_INT * 2. > In that case, hv < 0 can just mean that we have a uint128_t > (or whatever) whose high bit happens to be set. Well, according to the spec, one cannot use CONST_DOUBLE to represent a uint128 value with the high bit set. The C frontend type plays this game, but they can, because they track the type with the constant the the values of the constant are interpreted exclusively in the context of the type. Since we don't have the unsigned bit, we can't, so, either, they are speced to be values on their own, or values dependent upon some external notion. By changing the spec to say sign extending, we mean if the high bit is set, the value is negative. If this is the wrong value for a uint value, then that representation cannot be used. The only solution is to use a different representation (CONST_QUAD, CONST_UDOUBLE or something else). I'm thought about that routine some more, and it is just totally broken. I've changed the code to: /* We should never get a negative number. */ gcc_assert (hv >= 0); if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT) hv = 0, lv &= GET_MODE_MASK (op_mode); negative numbers are bad, period, this badness, doesn't depend upon anything other than the value being negative. The use of hv = 0, can only be done with PRECISIONs less than or equal to HOST_BITS_PER_WIDE_INT. Wider than this, and some of hv would be wiped out, which would be wrong. Thoughts? > OK with those changes as far as the RTL optimisations go. Not quite. Need to resolve the point just above. But, yea, we are getting very close now. I'm switched over to a patch for trunk (from 4.6.0) and added the ChangeLog. The switch to GET_MODE_PRECISION above was one change, for example. The rest fit in nicely, the arg to expand_shift was updated slightly to match trunk. > I can't approve the immed_double_const change itself. Sounds like Richard G would be > willing though. I've bundled in the patch that started this all into this version. I'll look forward to his review. * doc/rtl.texi (const_double): Document as sign-extending. * expmed.c (expand_mult): Ensure we don't use shift incorrectly. * emit-rtl.c (immed_double_int_const): Refine to state the value is signed. * simplify-rtx.c (mode_signbit_p): Add a fixme for wider than CONST_DOUBLE integers. (simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no negative values are converted. Fix conversions bigger than HOST_BITS_PER_WIDE_INT. (simplify_binary_operation_1): Ensure we don't use shift incorrectly. (simplify_immed_subreg): Sign-extend CONST_DOUBLEs. * explow.c (plus_constant_mode): Add. (plus_constant): Implement with plus_constant_mode. * rtl.h (plus_constant_mode): Add. Index: doc/rtl.texi =================================================================== --- doc/rtl.texi (revision 185706) +++ doc/rtl.texi (working copy) @@ -1510,7 +1510,9 @@ Represents either a floating-point const integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT} bits but small enough to fit within twice that number of bits (GCC does not provide a mechanism to represent even larger constants). In -the latter case, @var{m} will be @code{VOIDmode}. +the latter case, @var{m} will be @code{VOIDmode}. For integral values +the value is a signed value, meaning the top bit of +@code{CONST_DOUBLE_HIGH} is a sign bit. @findex CONST_DOUBLE_LOW If @var{m} is @code{VOIDmode}, the bits of the value are stored in Index: expmed.c =================================================================== --- expmed.c (revision 185706) +++ expmed.c (working copy) @@ -3135,8 +3135,10 @@ expand_mult (enum machine_mode mode, rtx { int shift = floor_log2 (CONST_DOUBLE_HIGH (op1)) + HOST_BITS_PER_WIDE_INT; - return expand_shift (LSHIFT_EXPR, mode, op0, - shift, target, unsignedp); + if (shift < 2 * HOST_BITS_PER_WIDE_INT - 1 + || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT) + return expand_shift (LSHIFT_EXPR, mode, op0, + shift, target, unsignedp); } } Index: emit-rtl.c =================================================================== --- emit-rtl.c (revision 185706) +++ emit-rtl.c (working copy) @@ -517,7 +517,8 @@ immed_double_int_const (double_int i, en /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair of ints: I0 is the low-order word and I1 is the high-order word. - Do not use this routine for non-integer modes; convert to + The value is a signed value, with the high bit of i1 being the sign + bit. Do not use this routine for non-integer modes; convert to REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ rtx @@ -531,10 +532,9 @@ immed_double_const (HOST_WIDE_INT i0, HO 1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use gen_int_mode. - 2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of - the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only - from copies of the sign bit, and sign of i0 and i1 are the same), then - we return a CONST_INT for i0. + 2) If the value of the integer fits into HOST_WIDE_INT anyway + (i.e., i1 consists only from copies of the sign bit, and sign + of i0 and i1 are the same), then we return a CONST_INT for i0. 3) Otherwise, we create a CONST_DOUBLE for i0 and i1. */ if (mode != VOIDmode) { @@ -546,8 +546,6 @@ immed_double_const (HOST_WIDE_INT i0, HO if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) return gen_int_mode (i0, mode); - - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); } /* If this integer fits in one word, return a CONST_INT. */ Index: simplify-rtx.c =================================================================== --- simplify-rtx.c (revision 185706) +++ simplify-rtx.c (working copy) @@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, width -= HOST_BITS_PER_WIDE_INT; } else + /* FIXME: We don't yet have a representation for wider modes. */ return false; if (width < HOST_BITS_PER_WIDE_INT) @@ -1355,16 +1356,10 @@ simplify_const_unary_operation (enum rtx else lv = CONST_DOUBLE_LOW (op), hv = CONST_DOUBLE_HIGH (op); - if (op_mode == VOIDmode) - { - /* We don't know how to interpret negative-looking numbers in - this case, so don't try to fold those. */ - if (hv < 0) - return 0; - } - else if (GET_MODE_PRECISION (op_mode) >= HOST_BITS_PER_WIDE_INT * 2) - ; - else + /* We should never get a negative number. */ + gcc_assert (hv >= 0); + + if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT) hv = 0, lv &= GET_MODE_MASK (op_mode); REAL_VALUE_FROM_UNSIGNED_INT (d, lv, hv, mode); @@ -1718,7 +1713,7 @@ simplify_const_unary_operation (enum rtx else if (GET_CODE (op) == CONST_DOUBLE && SCALAR_FLOAT_MODE_P (GET_MODE (op)) && GET_MODE_CLASS (mode) == MODE_INT - && width <= 2*HOST_BITS_PER_WIDE_INT && width > 0) + && width <= 2 * HOST_BITS_PER_WIDE_INT && width > 0) { /* Although the overflow semantics of RTL's FIX and UNSIGNED_FIX operators are intentionally left unspecified (to ease implementation @@ -1783,7 +1778,7 @@ simplify_const_unary_operation (enum rtx return const0_rtx; /* Test against the unsigned upper bound. */ - if (width == 2*HOST_BITS_PER_WIDE_INT) + if (width == 2 * HOST_BITS_PER_WIDE_INT) { th = -1; tl = -1; @@ -2380,7 +2375,9 @@ simplify_binary_operation_1 (enum rtx_co || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT) && GET_MODE (op0) == mode && CONST_DOUBLE_LOW (trueop1) == 0 - && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0) + && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0 + && (val < 2 * HOST_BITS_PER_WIDE_INT - 1 + || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)) return simplify_gen_binary (ASHIFT, mode, op0, GEN_INT (val + HOST_BITS_PER_WIDE_INT)); @@ -5189,6 +5186,7 @@ simplify_immed_subreg (enum machine_mode case CONST_DOUBLE: if (GET_MODE (el) == VOIDmode) { + unsigned char extend = 0; /* If this triggers, someone should have generated a CONST_INT instead. */ gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT); @@ -5201,10 +5199,11 @@ simplify_immed_subreg (enum machine_mode = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT); i += value_bit; } - /* It shouldn't matter what's done here, so fill it with - zero. */ + + if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT - 1)) + extend = -1; for (; i < elem_bitsize; i += value_bit) - *vp++ = 0; + *vp++ = extend; } else { Index: explow.c =================================================================== --- explow.c (revision 185706) +++ explow.c (working copy) @@ -74,14 +74,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enu return c; } -/* Return an rtx for the sum of X and the integer C. */ +/* Return an rtx for the sum of X and the integer C, given that X has + mode MODE. This routine should be used instead of plus_constant + when they want to ensure that addition happens in a particular + mode, which is necessary when x can be a VOIDmode CONST_INT or + CONST_DOUBLE and the width of the constant is different from the + width of the expression. */ +/* TODO: All callers of plus_constant should migrate to this routine, + and once they do, we can assert that mode is not VOIDmode. */ rtx -plus_constant (rtx x, HOST_WIDE_INT c) +plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c) { RTX_CODE code; rtx y; - enum machine_mode mode; rtx tem; int all_constant = 0; @@ -91,12 +97,26 @@ plus_constant (rtx x, HOST_WIDE_INT c) restart: code = GET_CODE (x); - mode = GET_MODE (x); y = x; switch (code) { case CONST_INT: + if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT) + { + unsigned HOST_WIDE_INT l1 = INTVAL (x); + HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT - 1)) ? -1 : 0; + unsigned HOST_WIDE_INT l2 = c; + HOST_WIDE_INT h2 = c < 0 ? -1 : 0; + unsigned HOST_WIDE_INT lv; + HOST_WIDE_INT hv; + + if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false)) + gcc_unreachable (); + + return immed_double_const (lv, hv, VOIDmode); + } + return GEN_INT (INTVAL (x) + c); case CONST_DOUBLE: @@ -104,11 +124,14 @@ plus_constant (rtx x, HOST_WIDE_INT c) unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x); HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x); unsigned HOST_WIDE_INT l2 = c; - HOST_WIDE_INT h2 = c < 0 ? ~0 : 0; + HOST_WIDE_INT h2 = c < 0 ? -1 : 0; unsigned HOST_WIDE_INT lv; HOST_WIDE_INT hv; - add_double (l1, h1, l2, h2, &lv, &hv); + if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false)) + /* Sorry, we have no way to represent overflows this wide. + To fix, add constant support wider than CONST_DOUBLE. */ + gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT); return immed_double_const (lv, hv, VOIDmode); } @@ -120,10 +143,8 @@ plus_constant (rtx x, HOST_WIDE_INT c) if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0))) { - tem - = force_const_mem (GET_MODE (x), - plus_constant (get_pool_constant (XEXP (x, 0)), - c)); + tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c); + tem = force_const_mem (GET_MODE (x), tem); if (memory_address_p (GET_MODE (tem), XEXP (tem, 0))) return tem; } @@ -142,31 +163,17 @@ plus_constant (rtx x, HOST_WIDE_INT c) break; case PLUS: - /* The interesting case is adding the integer to a sum. - Look for constant term in the sum and combine - with C. For an integer constant term, we make a combined - integer. For a constant term that is not an explicit integer, - we cannot really combine, but group them together anyway. - - Restart or use a recursive call in case the remaining operand is - something that we handle specially, such as a SYMBOL_REF. + /* The interesting case is adding the integer to a sum. Look + for constant term in the sum and combine with C. For an + integer constant term or a constant term that is not an + explicit integer, we combine or group them together anyway. We may not immediately return from the recursive call here, lest all_constant gets lost. */ - if (CONST_INT_P (XEXP (x, 1))) + if (CONSTANT_P (XEXP (x, 1))) { - c += INTVAL (XEXP (x, 1)); - - if (GET_MODE (x) != VOIDmode) - c = trunc_int_for_mode (c, GET_MODE (x)); - - x = XEXP (x, 0); - goto restart; - } - else if (CONSTANT_P (XEXP (x, 1))) - { - x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c)); + x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c)); c = 0; } else if (find_constant_term_loc (&y)) @@ -176,7 +183,7 @@ plus_constant (rtx x, HOST_WIDE_INT c) rtx copy = copy_rtx (x); rtx *const_loc = find_constant_term_loc (©); - *const_loc = plus_constant (*const_loc, c); + *const_loc = plus_constant_mode (mode, *const_loc, c); x = copy; c = 0; } @@ -196,6 +203,14 @@ plus_constant (rtx x, HOST_WIDE_INT c) else return x; } + +/* Return an rtx for the sum of X and the integer C. */ + +rtx +plus_constant (rtx x, HOST_WIDE_INT c) +{ + return plus_constant_mode (GET_MODE (x), x, c); +} /* If X is a sum, return a new sum like X but lacking any constant terms. Add all the removed constant terms into *CONSTPTR. Index: rtl.h =================================================================== --- rtl.h (revision 185706) +++ rtl.h (working copy) @@ -1644,6 +1644,7 @@ extern int ceil_log2 (unsigned HOST_WIDE /* In explow.c */ extern HOST_WIDE_INT trunc_int_for_mode (HOST_WIDE_INT, enum machine_mode); extern rtx plus_constant (rtx, HOST_WIDE_INT); +extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT); /* In rtl.c */ extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);