On Thu, Oct 18, 2012 at 2:52 PM, Kenneth Zadeck <zad...@naturalbridge.com> wrote: > > On 10/18/2012 06:22 AM, Richard Biener wrote: >> >> On Wed, Oct 17, 2012 at 11:47 PM, Kenneth Zadeck >> <zad...@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 > >