From patchwork Wed Mar 21 21:35:40 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Stump X-Patchwork-Id: 148068 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 2749BB6F6E for ; Thu, 22 Mar 2012 08:36:11 +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=1332970572; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Subject:Mime-Version:Content-Type:From:In-Reply-To:Date:Cc: Content-Transfer-Encoding:Message-Id:References:To:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=xE04VfDPQy7tF2Ahj0zhWMo63EA=; b=gWi36HjpzEfx6bdggfToNGMuKqsQqtwQHREj3jrRuJ+yC1Yj2arGq19gf1Jb3t 5eCp0wgvbneq4PF6m8ym0BXxDg41exAgqJT5yW60//ip9v3v0jqyyQyxAX8nG2aO N23iqAKimdY8XVpTA+sa7U/TO/bhD38CZkaMaQVAGeL34= 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:Content-Transfer-Encoding:Message-Id:References:To:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=t5NKP1QoKUHzVKSuGpr5Mb8d+SIFga9ARYjdt5tt0tG7l7gtmScEoRCZ/JveWV nFvMG6rZ58/BLDeAQ0apto1XePgv65DOBVeU4L1i6//ah1aHKxtLpdKl8/8KRnWi 7krLtWgVXQiq1a3qWVScheCSjxvSfH6hSLGjX1Y5bZru8=; Received: (qmail 28425 invoked by alias); 21 Mar 2012 21:36:06 -0000 Received: (qmail 27969 invoked by uid 22791); 21 Mar 2012 21:36:02 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from qmta15.westchester.pa.mail.comcast.net (HELO qmta15.westchester.pa.mail.comcast.net) (76.96.59.228) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 21 Mar 2012 21:35:44 +0000 Received: from omta07.westchester.pa.mail.comcast.net ([76.96.62.59]) by qmta15.westchester.pa.mail.comcast.net with comcast id oMHw1i0061GhbT85FMbk6E; Wed, 21 Mar 2012 21:35:44 +0000 Received: from up.mrs.kithrup.com ([24.4.193.8]) by omta07.westchester.pa.mail.comcast.net with comcast id oMbi1i00u0BKwT43TMbjkr; Wed, 21 Mar 2012 21:35:44 +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: Wed, 21 Mar 2012 14:35:40 -0700 Cc: gcc-patches Patches Message-Id: 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 21, 2012, at 6:17 AM, Richard Sandiford wrote: > Sounds good. Cool, a path forward. > For this I think we should make plus_constant a wrapper: Ah, yes, much better, thanks. I'd expanded the comments on plus_constant_mode so that one might have a better idea why the code is that way and put in a TODO, so that people have an idea of what direction the code wants to move. > I don't think it's a good idea to punt to a PLUS though. Fixed, thanks for the code. > Nicely, add_double returns true on overflow, so I think > we should replace the punt with: Ah, yes, nice, fixed. > For this I think we should change the recursive CONSTANT_P call > to use plus_constant_mode Done. > and we can remove the special CONST_INT case. Ok, ah, yes, because the recursive call will already combine them, done. >> if (width < HOST_BITS_PER_WIDE_INT) >> val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1; >> + /* FIXME: audit for TImode and wider correctness. */ >> return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1)); > > We've already returned false in that case though. Ah, I saw it, but missed it anyway, thanks for the pointer, fixed. > I'm happy for this function to be left as-is, but we could instead add a comment like: > > /* FIXME: We don't yet have a representation for wider modes. */ Done. >> @@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode, >> return 0; >> } >> else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2) >> - ; >> + return 0; >> else >> hv = 0, lv &= GET_MODE_MASK (op_mode); > > Should keep the current behaviour for == HOST_BITS_PER_WIDE_INT * 2. > I'd slightly prefer an assert rather than a "return false", but I won't > argue if another maintainer agrees with the return. Ah, yes, I agree an assert would be better, as really, no one should ask for an unsigned conversion to floating point on a value that is negative, more likely it is just a spot we missed adding an assert to earlier and probably wants a larger constant that can represent a large unsigned number. Fixed. > This is changing the real case, where sign extension doesn't make > much sense. I'm not sure I followed. Do you want me to remove the change for the second case, leave it as it, or something else? I've left it as I had modified it. > I think the expand_mult CONST_DOUBLE code needs changing Agreed. Fixed. I preserve the code for smaller modes, and for small enough shift amounts. 1<<67 for example, or any mode, is still handled. For large enough things, we just don't return the shift. > Same for: Fixed, in same way as previous case. > simplify_const_unary_operation needs to check for overflow > when handling 2-HWI arithmetic, since we can no longer assume > that the result is <= 2 HWIs in size. E.g.: > > case NEG: > neg_double (l1, h1, &lv, &hv); > break; > > should probably be: > > case NEG: > if (neg_double (l1, h1, &lv, &hv)) > gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT); > break; Are you talking about the block of code inside: /* We can do some operations on integer CONST_DOUBLEs. Also allow for a DImode operation on a CONST_INT. */ else if (GET_MODE (op) == VOIDmode && width <= HOST_BITS_PER_WIDE_INT * 2 ? If so, we already know that the width is <= HOST_BITS_PER_WIDE_INT * 2. :-) Thanks for spotting the bits I missed. Current patch: diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texiindex de45a22..0c6dc45 100644 --- a/gcc/doc/rtl.texi +++ b/gcc/doc/rtl.texi @@ -1530,7 +1530,9 @@ Represents either a floating-point constant of mode @var{m} or an 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 diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index 78ddfc3..c0b24e4 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode) 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) { diff --git a/gcc/explow.c b/gcc/explow.c index 2fae1a1..c94ad25 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -73,14 +73,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enum machine_mode mode) 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 smaller than 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; @@ -90,12 +96,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: @@ -103,11 +123,16 @@ 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)) + if (GET_MODE_BITSIZE (mode) > 2*HOST_BITS_PER_WIDE_INT) + /* Sorry, we have no way to represent overflows this + wide. To fix, add constant support wider than + CONST_DOUBLE. */ + gcc_assert (0); return immed_double_const (lv, hv, VOIDmode); } @@ -121,8 +146,8 @@ plus_constant (rtx x, HOST_WIDE_INT c) { tem = force_const_mem (GET_MODE (x), - plus_constant (get_pool_constant (XEXP (x, 0)), - c)); + plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), + c)); if (memory_address_p (GET_MODE (tem), XEXP (tem, 0))) return tem; } @@ -141,31 +166,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)) @@ -175,7 +186,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; } @@ -195,6 +206,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. diff --git a/gcc/expmed.c b/gcc/expmed.c index 3507dad..2361b7e 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -3122,9 +3122,11 @@ expand_mult (enum machine_mode mode, rtx op0, rtx op1, rtx target, { int shift = floor_log2 (CONST_DOUBLE_HIGH (op1)) + HOST_BITS_PER_WIDE_INT; - return expand_shift (LSHIFT_EXPR, mode, op0, - build_int_cst (NULL_TREE, 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, + build_int_cst (NULL_TREE, shift), + target, unsignedp); } } diff --git a/gcc/rtl.h b/gcc/rtl.h index 66f2755..9d1a28e 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -1586,6 +1586,7 @@ extern int ceil_log2 (unsigned HOST_WIDE_INT); /* 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); diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c index ce4eab4..ff838a8 100644 --- a/gcc/simplify-rtx.c +++ b/gcc/simplify-rtx.c @@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x) 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) @@ -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); @@ -2214,7 +2210,9 @@ simplify_binary_operation_1 (enum rtx_code code, enum machine_mode mode, || 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)); @@ -4987,6 +4985,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op, 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); @@ -4999,13 +4998,15 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op, = 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 { + unsigned char extend = 0; long tmp[max_bitsize / 32]; int bitsize = GET_MODE_BITSIZE (GET_MODE (el)); @@ -5030,10 +5031,10 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op, *vp++ = tmp[ibase / 32] >> i % 32; } - /* 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; } break;