From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20051 invoked by alias); 20 Mar 2012 11:38:40 -0000 Received: (qmail 20042 invoked by uid 22791); 20 Mar 2012 11:38:38 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-yx0-f175.google.com (HELO mail-yx0-f175.google.com) (209.85.213.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 20 Mar 2012 11:38:13 +0000 Received: by yenm3 with SMTP id m3so6445626yen.20 for ; Tue, 20 Mar 2012 04:38:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.50.104.166 with SMTP id gf6mr8373530igb.35.1332243492339; Tue, 20 Mar 2012 04:38:12 -0700 (PDT) Received: by 10.42.140.196 with HTTP; Tue, 20 Mar 2012 04:38:12 -0700 (PDT) In-Reply-To: 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: Tue, 20 Mar 2012 11:38:00 -0000 Message-ID: Subject: Re: remove wrong code in immed_double_const From: Richard Guenther To: Richard Guenther , Mike Stump , gcc-patches Patches , rdsandiford@googlemail.com Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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/msg01343.txt.bz2 On Tue, Mar 20, 2012 at 11:49 AM, Richard Sandiford wrote: > Richard Guenther writes: >> On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump wro= te: >>> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >>>> Mike Stump writes: >>>>>> If we're going to remove the assert, we need to define stuff like >>>>>> that. >>>>> >>>>> Orthogonal. =A0The rest of the compiler defines what happens, it eith= er >>>>> is inconsistent, in which case it is by fiat, undefined, or it is >>>>> consistent, in which case that consistency defines it. =A0The compiler >>>>> is free to document this in a nice way, or do, what is usually done, >>>>> which is to assume everybody just knows what it does. =A0Anyway, my >>>>> point is, this routine doesn't define the data structure, and is >>>>> _completely_ orthogonal to your concern. =A0It doesn't matter if it z= ero >>>>> extends or sign extends or is inconsistent, has bugs, doesn't have >>>>> bugs, is documented, or isn't documented. =A0In every single one of >>>>> these cases, the code in the routine I am fixing, doesn't change. >>>>> That is _why_ it is orthogonal. =A0If it weren't, you'd be able to st= ate >>>>> a value for which is mattered. =A0You can't, which is why you are wro= ng. >>>>> If you think you are not wrong, please state a value for which it >>>>> matters how it is defined. >>>> >>>> immed_double_const and CONST_DOUBLE are currently >>>> only defined for 2 HOST_WIDE_INTs. >>> >>> I don't happen to share your view. =A0The routine is defined by documen= tation. =A0The documentation might exist in a .texi file, in this case ther= e is no texi file for immed_double_const I don't think, next up, it is defi= ned by the comments before the routine. =A0In this case, it isn't so define= d. >>> >>> The current definition reads: >>> >>> /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair >>> =A0 of ints: I0 is the low-order word and I1 is the high-order word. >>> =A0 Do not use this routine for non-integer modes; convert to >>> =A0 REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. =A0*/ >>> >>> which, is is fine, and I don't _want_ to change that definition of the = routine. =A0I can't fix it, because it isn't broken. =A0If it were, you wou= ld be able to state a case where the new code behaves in a manor inconsiste= nt with the definition, since there is none you cannot state one, and this = is _why_ you have failed to state such a case. =A0If you disagree, please s= tate the case. >>> >>> Now, if you review comment is, could you please update the comments in = the routine, I would just say, oh, sure: >>> >>> Index: emit-rtl.c >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- emit-rtl.c =A0(revision 184563) >>> +++ emit-rtl.c =A0(working copy) >>> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO >>> >>> =A0 =A0 =A01) If GET_MODE_BITSIZE (mode) <=3D HOST_BITS_PER_WIDE_INT, t= hen we use >>> =A0 =A0 =A0 =A0gen_int_mode. >>> - =A0 =A0 2) GET_MODE_BITSIZE (mode) =3D=3D 2 * HOST_BITS_PER_WIDE_INT,= but the value of >>> - =A0 =A0 =A0 the integer fits into HOST_WIDE_INT anyway (i.e., i1 cons= ists only >>> - =A0 =A0 =A0 from copies of the sign bit, and sign of i0 and i1 are th= e same), =A0then >>> - =A0 =A0 =A0 we return a CONST_INT for i0. >>> + =A0 =A0 2) If the value of the integer fits into HOST_WIDE_INT anyway >>> + =A0 =A0 =A0 (i.e., i1 consists only from copies of the sign bit, and = sign >>> + =A0 =A0 =A0 of i0 and i1 are the same), then we return a CONST_INT fo= r i0. >>> =A0 =A0 =A03) Otherwise, we create a CONST_DOUBLE for i0 and i1. =A0*/ >>> =A0 if (mode !=3D VOIDmode) >>> =A0 =A0 { >>> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >>> >>> =A0 =A0 =A0 if (GET_MODE_BITSIZE (mode) <=3D HOST_BITS_PER_WIDE_INT) >>> =A0 =A0 =A0 =A0return gen_int_mode (i0, mode); >>> - >>> - =A0 =A0 =A0gcc_assert (GET_MODE_BITSIZE (mode) =3D=3D 2 * HOST_BITS_P= ER_WIDE_INT); >>> =A0 =A0 } >>> >>> =A0 /* If this integer fits in one word, return a CONST_INT. =A0*/ >>> >>> >>> Sorry I missed it. =A0Now, on to CONST_DOUBLE. =A0It does appear in a t= exi file: >>> >>> >>> @findex const_double >>> @item (const_double:@var{m} @var{i0} @var{i1} @dots{}) >>> 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). =A0In >>> the latter case, @var{m} will be @code{VOIDmode}. >>> >>> @findex CONST_DOUBLE_LOW >>> If @var{m} is @code{VOIDmode}, the bits of the value are stored in >>> @var{i0} and @var{i1}. =A0@var{i0} is customarily accessed with the mac= ro >>> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. >>> >>> >>> Here again, I don't want to change the definition. =A0The current defin= ition applies and I am merely making the code conform to it. =A0It says tha= t CONST_DOUBLE is used when the _value_ of the constant is too large to fit= into HOST_BITS_PER_WIDE_INT bits. >>> >>> So, if you disagree with me, you will necessarily have to quote the def= inition you are using, explain what the words mean to you _and_ state a spe= cific case in which the code post modification doesn't not conform with the= existing definition. =A0You have failed yet again to do that. >>> >>> >>>> So, as good functions do, immed_double_const asserts that it is not be= ing used out of spec. >>> >>> This does not follow from the definition. =A00 is a value that fits int= o HOST_BITS_PER_WIDE_INT bits. =A0It is representable in 0 bits. =A0HOST_BI= TS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST= _BITS_PER_WIDE_INT bits. >>> >>>> You want to remove that restriction on immed_double_const and CONST_DO= UBLE. >>>> That is, you want to change their spec. =A0We should only do that if w= e define >>>> what the new semantics are. >>> >>> You're assuming a definition for CONST_DOUBLE that only exists in your = mind, instead, please refer to the actual definition in the .texi file. >> >> Btw, I agree with Mike here (quite obvious if you followed the old >> e-mail thread). > > I've no objection to moving the assert down to after the GEN_INT. > But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing. > (That is, if we remove the assert altogether, we effectively treat the > number as sign-extended if it happens to fit in a CONST_INT, and > zero-extended otherwise. Why do we treat it zero-extended otherwise? Because we use gen_int_mode for CONST_INTs, which sign-extends? Yes, if we'd have a partial integer mode we would not properly truncate/extend the double-int input. OTOH all double-ints should come from trees where the constants are already properly extended (not sure why we forcefully sign-extend CONST_INTs for modes that are smaller than a HWI). > =A0That kind of inconsistency seems wrong, > and could turn what is now an ICE into a wrong code bug.) > >> But as there is some disagreement here I leave approval of the patch wit= h the >> comment change to someone to break that tie ;) > > No need for that. =A0Clearly it's just me :-) =A0Please go ahead and appr= ove > whatever you think is right. Well, even if I cannot make sense of the assert (and the assert does not change the above observation of inconsistency wrt extending partial integer modes) I'm not familiar enough with RTL to be confident to approve this change ;) Richard. > Richard