remove wrong code in immed_double_const

[Richard Guenther]

On Tue, Mar 20, 2012 at 11:49 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Guenther <richard.guenther@gmail.com> writes:
>> On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote:
>>>> Mike Stump <mikestump@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.

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).

>  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.

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