From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23494 invoked by alias); 22 Mar 2012 10:16:52 -0000 Received: (qmail 23483 invoked by uid 22791); 22 Mar 2012 10:16:50 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_SV X-Spam-Check-By: sourceware.org Received: from mail-wi0-f179.google.com (HELO mail-wi0-f179.google.com) (209.85.212.179) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Mar 2012 10:16:17 +0000 Received: by wibhn6 with SMTP id hn6so421258wib.8 for ; Thu, 22 Mar 2012 03:16:16 -0700 (PDT) Received: by 10.180.95.1 with SMTP id dg1mr3708547wib.21.1332411376290; Thu, 22 Mar 2012 03:16:16 -0700 (PDT) Received: from richards-thinkpad.stglab.manchester.uk.ibm.com (gbibp9ph1--blueice2n1.emea.ibm.com. [195.212.29.75]) by mx.google.com with ESMTPS id e6sm4083883wix.8.2012.03.22.03.16.15 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 22 Mar 2012 03:16:15 -0700 (PDT) From: Richard Sandiford To: Mike Stump Mail-Followup-To: Mike Stump ,gcc-patches Patches , rdsandiford@googlemail.com Cc: gcc-patches Patches Subject: Re: remove wrong code in immed_double_const 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> Date: Thu, 22 Mar 2012 10:16:00 -0000 In-Reply-To: (Mike Stump's message of "Wed, 21 Mar 2012 14:35:40 -0700") Message-ID: 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: 2012-03/txt/msg01485.txt.bz2 Mike Stump writes: >> 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. Sorry, meant we should leave the svn version as it is. The new docs (rightly) only mention sign-extension for integer modes, so the comment about it not mattering for FP modes is still correct. (In principle I'd prefer to replace it with an assert, but that's a separate change. Nothing we're doing here should change whether the FP path gets executed.) >> 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 > > ? Heh. it seems so :-) Never mind that bit then. > 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. */ I think this should be s/is smaller than/is different from/. We're in trouble whenever the width of the HWIs is different from the width of the constant they represent. > @@ -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); 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) (note spacing). > @@ -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; > } 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); > 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) Missing spaces around binary operators. > @@ -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. I think it should be something like: 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 if (GET_MODE_BITSIZE (op_mode) <= HOST_BITS_PER_WIDE_INT) 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)); > Missing spaces around binary operators. OK with those changes as far as the RTL optimisations go. I'm happy with the rest too, but despite all this fuss, I can't approve the immed_double_const change itself. Sounds like Richard G would be willing though. Richard