From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7066 invoked by alias); 20 Mar 2012 10:50:18 -0000 Received: (qmail 7052 invoked by uid 22791); 20 Mar 2012 10:50:16 -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 X-Spam-Check-By: sourceware.org Received: from mail-we0-f175.google.com (HELO mail-we0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 20 Mar 2012 10:49:49 +0000 Received: by wera1 with SMTP id a1so7605452wer.20 for ; Tue, 20 Mar 2012 03:49:48 -0700 (PDT) Received: by 10.180.80.9 with SMTP id n9mr27760316wix.4.1332240588409; Tue, 20 Mar 2012 03:49:48 -0700 (PDT) Received: from richards-thinkpad.stglab.manchester.uk.ibm.com (gbibp9ph1--blueice3n2.emea.ibm.com. [195.212.29.84]) by mx.google.com with ESMTPS id gp8sm33113839wib.5.2012.03.20.03.49.47 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 20 Mar 2012 03:49:47 -0700 (PDT) From: Richard Sandiford To: Richard Guenther Mail-Followup-To: Richard Guenther ,Mike Stump , gcc-patches Patches , rdsandiford@googlemail.com Cc: Mike Stump , 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: Tue, 20 Mar 2012 10:50:00 -0000 In-Reply-To: (Richard Guenther's message of "Tue, 20 Mar 2012 11:31:48 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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/msg01337.txt.bz2 Richard Guenther writes: > On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump wrot= e: >> 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. =C2=A0The rest of the compiler defines what happens, it ei= ther >>>> is inconsistent, in which case it is by fiat, undefined, or it is >>>> consistent, in which case that consistency defines it. =C2=A0The compi= ler >>>> 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. =C2=A0Anyway, my >>>> point is, this routine doesn't define the data structure, and is >>>> _completely_ orthogonal to your concern. =C2=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. =C2=A0In every single one of >>>> these cases, the code in the routine I am fixing, doesn't change. >>>> That is _why_ it is orthogonal. =C2=A0If it weren't, you'd be able to = state >>>> a value for which is mattered. =C2=A0You can't, which is why you are w= rong. >>>> 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. =C2=A0The routine is defined by docum= entation. =C2=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= defined by the comments before the routine. =C2=A0In this case, it isn't s= o defined. >> >> The current definition reads: >> >> /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair >> =C2=A0 of ints: I0 is the low-order word and I1 is the high-order word. >> =C2=A0 Do not use this routine for non-integer modes; convert to >> =C2=A0 REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. =C2=A0*/ >> >> which, is is fine, and I don't _want_ to change that definition of the r= outine. =C2=A0I can't fix it, because it isn't broken. =C2=A0If it were, yo= u would be able to state a case where the new code behaves in a manor incon= sistent with the definition, since there is none you cannot state one, and = this is _why_ you have failed to state such a case. =C2=A0If you disagree, = please state the case. >> >> Now, if you review comment is, could you please update the comments in t= he 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 =C2=A0(revision 184563) >> +++ emit-rtl.c =C2=A0(working copy) >> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> =C2=A0 =C2=A0 =C2=A01) If GET_MODE_BITSIZE (mode) <=3D HOST_BITS_PER_WID= E_INT, then we use >> =C2=A0 =C2=A0 =C2=A0 =C2=A0gen_int_mode. >> - =C2=A0 =C2=A0 2) GET_MODE_BITSIZE (mode) =3D=3D 2 * HOST_BITS_PER_WIDE= _INT, but the value of >> - =C2=A0 =C2=A0 =C2=A0 the integer fits into HOST_WIDE_INT anyway (i.e.,= i1 consists only >> - =C2=A0 =C2=A0 =C2=A0 from copies of the sign bit, and sign of i0 and i= 1 are the same), =C2=A0then >> - =C2=A0 =C2=A0 =C2=A0 we return a CONST_INT for i0. >> + =C2=A0 =C2=A0 2) If the value of the integer fits into HOST_WIDE_INT a= nyway >> + =C2=A0 =C2=A0 =C2=A0 (i.e., i1 consists only from copies of the sign b= it, and sign >> + =C2=A0 =C2=A0 =C2=A0 of i0 and i1 are the same), then we return a CONS= T_INT for i0. >> =C2=A0 =C2=A0 =C2=A03) Otherwise, we create a CONST_DOUBLE for i0 and i1= . =C2=A0*/ >> =C2=A0 if (mode !=3D VOIDmode) >> =C2=A0 =C2=A0 { >> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> =C2=A0 =C2=A0 =C2=A0 if (GET_MODE_BITSIZE (mode) <=3D HOST_BITS_PER_WIDE= _INT) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0return gen_int_mode (i0, mode); >> - >> - =C2=A0 =C2=A0 =C2=A0gcc_assert (GET_MODE_BITSIZE (mode) =3D=3D 2 * HOS= T_BITS_PER_WIDE_INT); >> =C2=A0 =C2=A0 } >> >> =C2=A0 /* If this integer fits in one word, return a CONST_INT. =C2=A0*/ >> >> >> Sorry I missed it. =C2=A0Now, on to CONST_DOUBLE. =C2=A0It does appear i= n a texi 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). =C2=A0= In >> 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}. =C2=A0@var{i0} is customarily accessed with the m= acro >> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. >> >> >> Here again, I don't want to change the definition. =C2=A0The current def= inition applies and I am merely making the code conform to it. =C2=A0It say= s that CONST_DOUBLE is used when the _value_ of the constant is too large t= o fit into HOST_BITS_PER_WIDE_INT bits. >> >> So, if you disagree with me, you will necessarily have to quote the defi= nition you are using, explain what the words mean to you _and_ state a spec= ific case in which the code post modification doesn't not conform with the = existing definition. =C2=A0You have failed yet again to do that. >> >> >>> So, as good functions do, immed_double_const asserts that it is not bei= ng used out of spec. >> >> This does not follow from the definition. =C2=A00 is a value that fits i= nto HOST_BITS_PER_WIDE_INT bits. =C2=A0It is representable in 0 bits. =C2= =A0HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representab= le by HOST_BITS_PER_WIDE_INT bits. >> >>> You want to remove that restriction on immed_double_const and CONST_DOU= BLE. >>> That is, you want to change their spec. =C2=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 m= ind, 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. That 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 with= the > comment change to someone to break that tie ;) No need for that. Clearly it's just me :-) Please go ahead and approve whatever you think is right. Richard