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

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

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.

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