From: Richard Sandiford <rdsandiford@googlemail.com>
To: Richard Guenther <richard.guenther@gmail.com>
Cc: Mike Stump <mikestump@comcast.net>,
gcc-patches Patches <gcc-patches@gcc.gnu.org>
Subject: Re: remove wrong code in immed_double_const
Date: Tue, 20 Mar 2012 10:50:00 -0000 [thread overview]
Message-ID: <g44ntjh7hx.fsf@richards-thinkpad.stglab.manchester.uk.ibm.com> (raw)
In-Reply-To: <CAFiYyc1Yn8nvfqxwYQKhoBATG=QJg+tvWRK2aa1LTdY3+oHSkw@mail.gmail.com> (Richard Guenther's message of "Tue, 20 Mar 2012 11:31:48 +0100")
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. 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
next prev parent reply other threads:[~2012-03-20 10:50 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-16 21:54 Mike Stump
2012-03-16 22:04 ` Steven Bosscher
2012-03-17 1:03 ` Mike Stump
2012-03-17 7:37 ` Richard Sandiford
2012-03-18 0:29 ` Mike Stump
2012-03-18 10:16 ` Richard Sandiford
2012-03-18 16:35 ` Mike Stump
2012-03-19 21:44 ` Richard Sandiford
2012-03-19 23:31 ` Mike Stump
2012-03-20 10:32 ` Richard Guenther
2012-03-20 10:50 ` Richard Sandiford [this message]
2012-03-20 11:38 ` Richard Guenther
2012-03-20 12:27 ` Richard Sandiford
2012-03-20 12:47 ` Richard Guenther
2012-03-20 13:55 ` Michael Matz
2012-03-20 20:44 ` Mike Stump
2012-03-21 13:47 ` Michael Matz
2012-03-21 17:01 ` Mike Stump
2012-03-22 13:16 ` Michael Matz
2012-03-22 18:37 ` Mike Stump
2012-03-20 19:41 ` Mike Stump
2012-03-21 1:01 ` Mike Stump
2012-03-21 13:17 ` Richard Sandiford
2012-03-21 21:36 ` Mike Stump
2012-03-22 10:16 ` Richard Sandiford
2012-03-22 10:25 ` Richard Sandiford
2012-03-22 20:28 ` Mike Stump
2012-03-23 10:02 ` Richard Sandiford
2012-03-26 19:14 ` Mike Stump
2012-03-26 20:04 ` Richard Sandiford
2012-03-26 23:57 ` Mike Stump
2012-04-04 21:07 ` Mike Stump
2012-03-22 14:12 ` Michael Matz
2012-03-22 18:55 ` Mike Stump
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=g44ntjh7hx.fsf@richards-thinkpad.stglab.manchester.uk.ibm.com \
--to=rdsandiford@googlemail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=mikestump@comcast.net \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).