Re: remove wrong code in immed_double_const

[Richard Guenther <richard.guenther@gmail.com>]

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).
But as there is some disagreement here I leave approval of the patch with the
comment change to someone to break that tie ;)

Thanks,
Richard.