On Thu, Oct 18, 2012 at 4:00 PM, Kenneth Zadeck <zad...@naturalbridge.com> wrote: > you know richi, i did not know who i was actually talking to. i said who > is this richard beiner person and then i saw the email address.
;) > On 10/18/2012 08:58 AM, Richard Biener wrote: >> >> 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. > > these are bugs in the changelog, not the code. new changelog included. > >>> >>>> 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. > > Note that tree_fits_hwi_p does check. it just takes a parameter to say if > it wants signed or unsigned checking (it is the old host_integerp, > repackaged). You really do need this function as it is for the 4 or 5 > places it is called. The parameter to it is typically, but not always, the > sign of the type of the int cst being passed to it. > > it is the tree_to_hwi that is unchecked. Most of the places are identified > with comments. This survives the changelog. (i happen to be in the group > of people that think changelogs are useless, and that we should do a better > job of commenting the code.) > > I do not know if this is sloppyness or not, but the signedness that is > checked rarely matches the sign of the variable that the value is assigned. > I found this quite frustrating when i was making the patch but this kind of > thing is common in code where the original writer "knew what (s)he was > doing." Unless you are doing comparisons or shifting, the signedness of > target does not really make much difference. > > if you want me to change the sequences of explicit checking and unchecked > access to explicit checking followed by a checked access, then i am happy to > do this. Disclaimer: not looking at the patch (again). The existing tree_low_cst function performs checking, so tree_to_hwi should as well. I don't think mismatch of signedness of the variable assigned to with the sign we use for hwi extraction is any good. C++ isn't type-safe here for the return value but if we'd use a reference as return slot we could make it so ... (in exchange for quite some ugliness IMNSHO): void tree_to_shwi (const_tree tree, HOST_WIDE_INT &hwi); vs. void tree_to_uhwi (const_tree tree, unsigned HOST_WIDE_INT &hwi); maybe more natural would be void hwi_from_tree (HOST_WIDE_INT &hwi, const_tree tree); void hwi_from_tree (unsigned HOST_WIDE_INT &hwi, const_tree tree); let the C++ bikeshedding begin! (the point is to do appropriate checking for a conversion of (INTEGER_CST) tree to HOST_WIDE_INT vs. unsigned HOST_WIDE_INT) No, I don't want you to do the above transform with this patch ;) Thanks, Richard. > kenny > >> 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 >>> >>> >
