Richard Guenther <richard.guent...@gmail.com> writes: > On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikest...@comcast.net> wrote: >> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >>> Mike Stump <mikest...@comcast.net> writes: >>>>> If we're going to remove the assert, we need to define stuff like >>>>> that. >>>> >>>> Orthogonal. The 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. The 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. Anyway, my >>>> point is, this routine doesn't define the data structure, and is >>>> _completely_ orthogonal to your concern. It 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. In every single one of >>>> these cases, the code in the routine I am fixing, doesn't change. >>>> That is _why_ it is orthogonal. If it weren't, you'd be able to state >>>> a value for which is mattered. You 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. The routine is defined by documentation. >> The 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. In 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 >> of ints: I0 is the low-order word and I1 is the high-order word. >> Do not use this routine for non-integer modes; convert to >> REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ >> >> which, is is fine, and I don't _want_ to change that definition of the >> routine. I can't fix it, because it isn't broken. If 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. If you disagree, please state 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 >> =================================================================== >> --- emit-rtl.c (revision 184563) >> +++ emit-rtl.c (working copy) >> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> 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) >> { >> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) >> return gen_int_mode (i0, mode); >> - >> - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); >> } >> >> /* If this integer fits in one word, return a CONST_INT. */ >> >> >> Sorry I missed it. Now, on to CONST_DOUBLE. It does appear in 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). 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}. @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. The current definition >> applies and I am merely making the code conform to it. It says that >> 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 >> definition you are using, explain what the words mean to you _and_ state a >> specific case in which the code post modification doesn't not conform with >> the existing definition. You have failed yet again to do that. >> >> >>> So, as good functions do, immed_double_const asserts that it is not being >>> used out of spec. >> >> This does not follow from the definition. 0 is a value that fits into >> HOST_BITS_PER_WIDE_INT bits. It is representable in 0 bits. >> HOST_BITS_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_DOUBLE. >>> That is, you want to change their spec. We 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 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. 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