Re: wide int patch #6: Replacement of hwi extraction from int-csts.
On 10/19/12, Richard Biener richard.guent...@gmail.com wrote: 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) We could add conversion operators to achieve the effect. However, we probably don't want to do so until we can make them explicit. Unfortunately, explicit conversion operators are not available until C++11. No, I don't want you to do the above transform with this patch ;) -- Lawrence Crowl
Re: wide int patch #6: Replacement of hwi extraction from int-csts.
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
Re: wide int patch #6: Replacement of hwi extraction from int-csts.
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 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. 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
Re: wide int patch #6: Replacement of hwi extraction from int-csts.
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. 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
Re: wide int patch #6: Replacement of hwi extraction from int-csts.
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
Re: wide int patch #6: Replacement of hwi extraction from int-csts.
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. 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
