wide int patch #6: Replacement of hwi extraction from int-csts.

[Richard Biener]

On Thu, Oct 18, 2012 at 2:52 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
>
> On 10/18/2012 06:22 AM, Richard Biener wrote:
>>
>> On Wed, Oct 17, 2012 at 11:47 PM, Kenneth Zadeck
>> <zadeck@naturalbridge.com> wrote:
>>>
>>> Richi,
>>>
>>> I apologize for the size of this patch, but it only does one very small
>>> thing, it is just that it does it all over the middle end.
>>>
>>> This patch introduces a new api for extracting a signed or unsigned hwi
>>> from
>>> an integer cst.  There had been an abi for doing this, but it has some
>>> problems that were fixed here, and it was only used sometimes.
>>>
>>> The new abi consists of 6 functions, three for testing if the constant in
>>> the int cst will fit and three for actually pulling out the value.
>>>
>>> The ones for testing are tree_fits_uhwi_p, tree_fits_shwi_p,
>>> tree_fits_hwi_p.   The first two of these are unsigned and signed
>>> versions,
>>> and the second one takes a boolean parameter which is true if the value
>>> is
>>> positive.   This replaces the host_integerp which basically has the same
>>> functionality of tree_fits_hwi_p.   The reason for changing this is that
>>> there were about 400 calls to host_integerp and all but 4 of them took a
>>> constant parameter. These names are chosen to be similar to the similar
>>> functions in wide-int and are more mnemonic that the existing name since
>>> they use the more readable u and s prefixes that a lot of other places
>>> do.
>>>
>>> On the accessing side, there is tree_to_uhwi, tree_to_shwi, and
>>> tree_to_hwi.
>>> The first two do checking when checking is enabled. The last does no
>>> checking.
>>
>> Just a quick note here - the changelog mentions tree_low_cst (as new
>> function!?) but not host_integerp.  You should probably get rid of both,
>> otherwise uses will creap back as people are familiar with them
>> (I'm not sure these changes for consistency are always good ...)
>
> i will fix this.
>
>> I don't like that tree_to_hwi does not do any checking.  In fact I don't
>> like that it _exists_, after all it has a return type which signedness
>> does not magically change.  Unchecked conversions should use
>> TREE_LOW_CST.
>
> the idea is that when wide-int goes in, there is actually no
> TREE_INT_CST_LOW.   The concept of low and high go out the door. the int-cst
> will have an array in it that is big enough to hold the value.
> so tree_to_hwi becomes a short hand for just accessing the lower element of
> the array.
>
> you could argue that you should say tree_fits_uhwi_p followed by
> tree_to_uhwi (and the same for signed).   This is an easy fix.   it just
> seemed a little redundant.

Well, if you want raw access to the lower element (when do you actually
want that, when not in wide-int.c/h?) ... you still need to care about the
signedness of the result.  And tree_fits_uhwi_p does not return the
same as tree_fits_shwi_p all the time.

I don't see any goodness in tree_to_hwi nor tree_fits_hwi really.  Because
if you just access the lower word then that still has a sign (either
HOST_WIDE_INT or unsigned HOST_WIDE_INT).  We should get rid
of those places - can you enumerate them?  I think you said it was just
a few callers with variable signedness argument.

Richard.

> I should also point out that about 2/3 if this patch is going to die as the
> rest of the wide int stuff goes in.   But i did not want to submit a patch
> that only converted 1/3 of the cases.   The truth is that most of these
> places where you are doing this conversion are just because the people were
> too lazy to do the math at the full precision of the double int.
>
>>
>> Thus, my 2 cents before I really look at the patch (which will likely
>> be next week only, so maybe you can do a followup with the above
>> suggestions).
>>
>> Thanks,
>> Richard.
>>
>>> it is expected normally, the unchecked accesses follows an explicit
>>> check,
>>> or if there is no explicit check, then one of the checked ones follows.
>>> This is always not the case for places that just want to look at the
>>> bottom
>>> bits of some large number, such as several places that compute alignment.
>>> These just use the naked unchecked access.
>>>
>>> There were a lot of places that either did no checking or did the
>>> checking
>>> inline.   This patch tracks down all of those places and they now use the
>>> same abi.
>>>
>>> There are three places where i found what appear to be bugs in the code.
>>> These are places where the existing code did an unsigned check and then
>>> followed it with a signed access (or visa versa). These places are marked
>>> with comments that contain the string "THE NEXT LINE IS POSSIBLY WRONG".
>>> With some guidance from the reviewer, i will fix these are remove the
>>> unacceptable comments.
>>>
>>> Aside from that, this patch is very very boring because it just makes
>>> this
>>> one transformation. Look for interesting stuff in tree.h.   Everything
>>> else
>>> is just forcing everywhere to use a single abi.
>>>
>>> This patch could go on with a little work without the other 5 patches or
>>> it
>>> can wait for them.
>>>
>>> kenny
>
>