From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9297 invoked by alias); 20 Mar 2012 10:32:25 -0000 Received: (qmail 9283 invoked by uid 22791); 20 Mar 2012 10:32:21 -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-iy0-f175.google.com (HELO mail-iy0-f175.google.com) (209.85.210.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 20 Mar 2012 10:31:49 +0000 Received: by iaag37 with SMTP id g37so11531039iaa.20 for ; Tue, 20 Mar 2012 03:31:48 -0700 (PDT) MIME-Version: 1.0 Received: by 10.50.187.165 with SMTP id ft5mr1260684igc.25.1332239508631; Tue, 20 Mar 2012 03:31:48 -0700 (PDT) Received: by 10.42.140.196 with HTTP; Tue, 20 Mar 2012 03:31:48 -0700 (PDT) In-Reply-To: <0A5CBD0C-FC94-4637-B230-1A83372DE91A@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> Date: Tue, 20 Mar 2012 10:32:00 -0000 Message-ID: Subject: Re: remove wrong code in immed_double_const From: Richard Guenther To: Mike Stump Cc: Richard Sandiford , gcc-patches Patches 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/msg01335.txt.bz2 On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump wrote: > 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 either >>> 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 zero >>> 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 state >>> a value for which is mattered. =A0You can't, which is why you are wrong. >>> 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 documenta= tion. =A0The documentation might exist in a .texi file, in this case there = is no texi file for immed_double_const I don't think, next up, it is define= d by the comments before the routine. =A0In this case, it isn't so defined. > > 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 ro= utine. =A0I can't fix it, because it isn't broken. =A0If it were, you would= be able to state a case where the new code behaves in a manor inconsistent= 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 sta= te the case. > > Now, if you review comment is, could you please update the comments in th= e 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, the= n we use > =A0 =A0 =A0 =A0gen_int_mode. > - =A0 =A0 2) GET_MODE_BITSIZE (mode) =3D=3D 2 * HOST_BITS_PER_WIDE_INT, b= ut the value of > - =A0 =A0 =A0 the integer fits into HOST_WIDE_INT anyway (i.e., i1 consis= ts only > - =A0 =A0 =A0 from copies of the sign bit, and sign of i0 and i1 are the = 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 si= gn > + =A0 =A0 =A0 of i0 and i1 are the same), then we return a CONST_INT for = 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_PER= _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 tex= i 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 macro > @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. > > > Here again, I don't want to change the definition. =A0The current definit= ion applies and I am merely making the code conform to it. =A0It says that = CONST_DOUBLE is used when the _value_ of the constant is too large to fit i= nto HOST_BITS_PER_WIDE_INT bits. > > So, if you disagree with me, you will necessarily have to quote the defin= ition you are using, explain what the words mean to you _and_ state a speci= fic case in which the code post modification doesn't not conform with the e= xisting definition. =A0You have failed yet again to do that. > > >> So, as good functions do, immed_double_const asserts that it is not bein= g used out of spec. > > This does not follow from the definition. =A00 is a value that fits into = HOST_BITS_PER_WIDE_INT bits. =A0It is representable in 0 bits. =A0HOST_BITS= _PER_WIDE_INT is zero or more, and by induction, is representable by HOST_B= ITS_PER_WIDE_INT bits. > >> You want to remove that restriction on immed_double_const and CONST_DOUB= LE. >> That is, you want to change their spec. =A0We should only do that if we = define >> what the new semantics are. > > You're assuming a definition for CONST_DOUBLE that only exists in your mi= nd, 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). But as there is some disagreement here I leave approval of the patch with t= he comment change to someone to break that tie ;) Thanks, Richard.