On Fri, Oct 19, 2012 at 12:59 PM, Kenneth Zadeck
<zad...@naturalbridge.com> wrote:
>
> On 10/19/2012 04:22 AM, Richard Biener wrote:
>>
>> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
>> <zad...@naturalbridge.com> wrote:
>>>
>>> This patch replaces all instances of INT_CST_LT and INT_CST_LT_UNSIGNED
>>> with
>>> the new functions tree_int_cst_lts_p and tree_int_cst_ltu_p. With the
>>> new
>>> implementation of int_cst these functions will be too big to do inline.
>>
>> These new function names are extremely confusing given that we already
>> have tree_int_cst_lt which does the right thing based on the signedness
>> of the INTEGER_CST trees.
>>
>> The whole point of the macros was to be inlined and you break that. That
>> is,
>> for example
>>
>> if (unsignedp && unsignedp0)
>> {
>> - min_gt = INT_CST_LT_UNSIGNED (primop1, minval);
>> - max_gt = INT_CST_LT_UNSIGNED (primop1, maxval);
>> - min_lt = INT_CST_LT_UNSIGNED (minval, primop1);
>> - max_lt = INT_CST_LT_UNSIGNED (maxval, primop1);
>> + min_gt = tree_int_cst_ltu_p (primop1, minval);
>> + max_gt = tree_int_cst_ltu_p (primop1, maxval);
>> + min_lt = tree_int_cst_ltu_p (minval, primop1);
>> + max_lt = tree_int_cst_ltu_p (maxval, primop1);
>> }
>> else
>> {
>> - min_gt = INT_CST_LT (primop1, minval);
>> - max_gt = INT_CST_LT (primop1, maxval);
>> - min_lt = INT_CST_LT (minval, primop1);
>> - max_lt = INT_CST_LT (maxval, primop1);
>> + min_gt = tree_int_cst_lts_p (primop1, minval);
>> ...
>>
>> could have just been
>>
>> min_gt = tree_int_cst_lt (primop1, minval);
>> ....
>>
>> without any sign check.
>>
>> So if you think you need to kill the inlined variants please use the
>> existing
>> tree_int_cst_lt instead.
>
> no, they could not have. tree_int_cst_lt uses the signedness of the type
> to determine how to do the comparison. These two functions, as the macros
> they replace, force the comparison to be done independent of the signedness
> of the type.
Well, looking at the surrounding code it's indeed non-obvious that this would
be a 1:1 transform. But then they should not compare _trees_ but instead
compare double-ints (or wide-ints).
That said, I still think having a tree_int_cst_lt[us]_p function is wrong.
tree INTEGER_CSTs have a sign. (apart from that opinion we have
tree_int_cst_lt and you introduce tree_int_cst_ltu_p - consistent
would be tree_int_cst_ltu).
> I do not know why we need to do this. I am just applying a plug compatible
> replacement here. I did not write this code, but I do not think that i can
> just do as you say here.
So use the double-int interface in the places you substituted your new
tree predicates. Yes, you'll have to touch that again when converting to
wide-int - but if those places really want to ignore the sign of the tree
they should not use a tree interface.
Richard.
> Kenny
>
>
>> Thanks,
>> Richard.
>>
>>> This is a small patch that has no prerequisites.
>>>
>>> Tested on x86-64.
>>>
>>> kenny
>
>
