diff mbox

wide int patch #7: Replacement of INT_CST_LT and INT_CST_LT_UNSIGNED

Message ID 50803D1A.8060702@naturalbridge.com
State New
Headers show

Commit Message

Kenneth Zadeck Oct. 18, 2012, 5:32 p.m. UTC
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.

This is a small patch that has no prerequisites.

Tested on x86-64.

kenny
2012-10-18  Kenneth Zadeck <zadeck@naturalbridge.com>
	* c-family/c-common.c (shorten_compare): Now uses
	tree_int_cst_ltu_p and tree_int_cst_lts_p.
	* cp/call.c (type_passed_as, convert_for_arg_passing): Ditto.
	* cp/class.c (walk_subobject_offsets, walk_subobject_offsets,
	end_of_class, include_empty_classes, layout_class_type): Ditto.
	* cp/decl.c (compute_array_index_type): Ditto.
	* fold-const.c (fold_relational_const): Ditto.
	* java/expr.c (build_field_ref): Ditto.
	* targhooks.c (default_cxx_get_cookie_size): Ditto.
	* tree-ssa-uninit.c (is_value_included_in): Ditto.
	* tree-vrp.c (operand_less_p): Ditto.
	* tree.c (tree_int_cst_lt): Ditto.
	(int_cst_sign_mask, tree_int_cst_lts_p)
	(tree_int_cst_ltu_p): New functions.
	* tree.h (INT_CST_LT, INT_CST_LT_UNSIGNED): Remove.

Comments

Richard Biener Oct. 19, 2012, 8:22 a.m. UTC | #1
On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
<zadeck@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.

Thanks,
Richard.

> This is a small patch that has no prerequisites.
>
> Tested on x86-64.
>
> kenny
Kenneth Zadeck Oct. 19, 2012, 10:59 a.m. UTC | #2
On 10/19/2012 04:22 AM, Richard Biener wrote:
> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
> <zadeck@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.

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.

Kenny

> Thanks,
> Richard.
>
>> This is a small patch that has no prerequisites.
>>
>> Tested on x86-64.
>>
>> kenny
Richard Biener Oct. 19, 2012, 11:13 a.m. UTC | #3
On Fri, Oct 19, 2012 at 12:59 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
>
> On 10/19/2012 04:22 AM, Richard Biener wrote:
>>
>> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
>> <zadeck@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
>
>
Kenneth Zadeck Oct. 19, 2012, 11:49 a.m. UTC | #4
On 10/19/2012 07:13 AM, Richard Biener wrote:
> On Fri, Oct 19, 2012 at 12:59 PM, Kenneth Zadeck
> <zadeck@naturalbridge.com> wrote:
>> On 10/19/2012 04:22 AM, Richard Biener wrote:
>>> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
>>> <zadeck@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).
This reply applies just as much to this patch as patch 6.
I morally agree with you 100%.    But the code does not agree with you.

On patch 6, there are about 450 places where we want to take the lower 
hwi worth of bits out of a int cst.    Of those, only 5 places use the 
function that allows the signedness to be passed in.   The rest make the 
signedness decision based on the local code and completely ignore any 
information in the type.    Of those 5 that do allow the signedness to 
be passed in, only three of them actually pass it in based on the 
signedness of the variable they are accessing.

I am sure that a lot of these are wrong.  But i could not tell you which 
are and which are not.
luckily, a lot of this will go away with the full wide-int code because 
i just do most of this math in the full precision so the issue never 
comes up.    But after i am finished, there will still be a fair number 
of places that do this.   (luckily, a large number of them are pulling 
the number out and comparing it to the precision of something, so this 
is likely to be harmless no matter how the code is written).

But to a large extent, you are shooting the messenger here, and not 
person who committed the crime.   I will be happy to add some comments 
to point the clients of these to the one that looks at the type.   In 
looking over the patch, the only obvious ones that could be changed are 
the ones in tree-ssa-uninit.c and the tree-vrp.c. The one in tree-vrp.c 
just looks like that the person writing the code did not know about 
tree_int_cst_lt and wrote the check out our himself.  (i will fix this 
in the tree-vrp patch that i am working on now. The one in 
tree-ssa-uniunit looks correct.

But beyond that, the rest are in the front ends and so i think that this 
as good as you get out of me.

Kenny

>> 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
>>
Richard Biener Oct. 19, 2012, 11:58 a.m. UTC | #5
On Fri, Oct 19, 2012 at 1:49 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
>
> On 10/19/2012 07:13 AM, Richard Biener wrote:
>>
>> On Fri, Oct 19, 2012 at 12:59 PM, Kenneth Zadeck
>> <zadeck@naturalbridge.com> wrote:
>>>
>>> On 10/19/2012 04:22 AM, Richard Biener wrote:
>>>>
>>>> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
>>>> <zadeck@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).
>
> This reply applies just as much to this patch as patch 6.
> I morally agree with you 100%.    But the code does not agree with you.
>
> On patch 6, there are about 450 places where we want to take the lower hwi
> worth of bits out of a int cst.    Of those, only 5 places use the function
> that allows the signedness to be passed in.   The rest make the signedness
> decision based on the local code and completely ignore any information in
> the type.    Of those 5 that do allow the signedness to be passed in, only
> three of them actually pass it in based on the signedness of the variable
> they are accessing.
>
> I am sure that a lot of these are wrong.  But i could not tell you which are
> and which are not.
> luckily, a lot of this will go away with the full wide-int code because i
> just do most of this math in the full precision so the issue never comes up.
> But after i am finished, there will still be a fair number of places that do
> this.   (luckily, a large number of them are pulling the number out and
> comparing it to the precision of something, so this is likely to be harmless
> no matter how the code is written).
>
> But to a large extent, you are shooting the messenger here, and not person
> who committed the crime.   I will be happy to add some comments to point the
> clients of these to the one that looks at the type.   In looking over the
> patch, the only obvious ones that could be changed are the ones in
> tree-ssa-uninit.c and the tree-vrp.c. The one in tree-vrp.c just looks like
> that the person writing the code did not know about tree_int_cst_lt and
> wrote the check out our himself.  (i will fix this in the tree-vrp patch
> that i am working on now. The one in tree-ssa-uniunit looks correct.
>
> But beyond that, the rest are in the front ends and so i think that this as
> good as you get out of me.

Well, if you transform bogus (by moral standards) code into other
bogus code the whole point of your patch is to exchange the names
of a set of macros / functions to another set of macros / functions.

I see no point in that then.

Leave broken code as-is.  The more often you touch broken code
and just mangle it in some way the harder it gets to get to the
point that would maybe reveal the real intent of the original code.

Sorry for the harsh words, but to take the example of
INT_CST_LT and INT_CST_LT_UNSIGNED -- those are remanents of
a world before double_ints existed.  All uses should have been
replaced by double_int usage by now; replacing them with something
tree-ish is the wrong way.  It might be the correct way if the tree
operands have the correct signedness - in which case we already
have tree_int_cst_lt for the task.  tree_int_cst_lt[us] is a perversion
(that is, wrong when viewed in isolation)!

Richard.

> Kenny
>
>
>>> 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
>>>
>>>
>
Kenneth Zadeck Oct. 19, 2012, 12:12 p.m. UTC | #6
On 10/19/2012 07:58 AM, Richard Biener wrote:
> On Fri, Oct 19, 2012 at 1:49 PM, Kenneth Zadeck
> <zadeck@naturalbridge.com> wrote:
>> On 10/19/2012 07:13 AM, Richard Biener wrote:
>>> On Fri, Oct 19, 2012 at 12:59 PM, Kenneth Zadeck
>>> <zadeck@naturalbridge.com> wrote:
>>>> On 10/19/2012 04:22 AM, Richard Biener wrote:
>>>>> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
>>>>> <zadeck@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).
>> This reply applies just as much to this patch as patch 6.
>> I morally agree with you 100%.    But the code does not agree with you.
>>
>> On patch 6, there are about 450 places where we want to take the lower hwi
>> worth of bits out of a int cst.    Of those, only 5 places use the function
>> that allows the signedness to be passed in.   The rest make the signedness
>> decision based on the local code and completely ignore any information in
>> the type.    Of those 5 that do allow the signedness to be passed in, only
>> three of them actually pass it in based on the signedness of the variable
>> they are accessing.
>>
>> I am sure that a lot of these are wrong.  But i could not tell you which are
>> and which are not.
>> luckily, a lot of this will go away with the full wide-int code because i
>> just do most of this math in the full precision so the issue never comes up.
>> But after i am finished, there will still be a fair number of places that do
>> this.   (luckily, a large number of them are pulling the number out and
>> comparing it to the precision of something, so this is likely to be harmless
>> no matter how the code is written).
>>
>> But to a large extent, you are shooting the messenger here, and not person
>> who committed the crime.   I will be happy to add some comments to point the
>> clients of these to the one that looks at the type.   In looking over the
>> patch, the only obvious ones that could be changed are the ones in
>> tree-ssa-uninit.c and the tree-vrp.c. The one in tree-vrp.c just looks like
>> that the person writing the code did not know about tree_int_cst_lt and
>> wrote the check out our himself.  (i will fix this in the tree-vrp patch
>> that i am working on now. The one in tree-ssa-uniunit looks correct.
>>
>> But beyond that, the rest are in the front ends and so i think that this as
>> good as you get out of me.
> Well, if you transform bogus (by moral standards) code into other
> bogus code the whole point of your patch is to exchange the names
> of a set of macros / functions to another set of macros / functions.
>
> I see no point in that then.
>
> Leave broken code as-is.  The more often you touch broken code
> and just mangle it in some way the harder it gets to get to the
> point that would maybe reveal the real intent of the original code.
>
> Sorry for the harsh words, but to take the example of
> INT_CST_LT and INT_CST_LT_UNSIGNED -- those are remanents of
> a world before double_ints existed.  All uses should have been
> replaced by double_int usage by now; replacing them with something
> tree-ish is the wrong way.  It might be the correct way if the tree
> operands have the correct signedness - in which case we already
> have tree_int_cst_lt for the task.  tree_int_cst_lt[us] is a perversion
> (that is, wrong when viewed in isolation)!
i do not know if this code is bogus.    i am not a front end person.
i just need the macros to go away.     And the conventions of the gcc 
say that we do not put real functions with upper case names.

i can of course, convert these to use wide-int and then use the 
wide_int::lts_p and ::ltu_p but that is just going to sweep the issue 
under the rug.   It does not get rid of the issue that in your mind 
these comparisons should be using the signedness of the constants them 
selves.

Kenny
> Richard.
>
>> Kenny
>>
>>
>>>> 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
>>>>
Richard Biener Oct. 19, 2012, 1:04 p.m. UTC | #7
On Fri, Oct 19, 2012 at 2:12 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
>
> On 10/19/2012 07:58 AM, Richard Biener wrote:
>>
>> On Fri, Oct 19, 2012 at 1:49 PM, Kenneth Zadeck
>> <zadeck@naturalbridge.com> wrote:
>>>
>>> On 10/19/2012 07:13 AM, Richard Biener wrote:
>>>>
>>>> On Fri, Oct 19, 2012 at 12:59 PM, Kenneth Zadeck
>>>> <zadeck@naturalbridge.com> wrote:
>>>>>
>>>>> On 10/19/2012 04:22 AM, Richard Biener wrote:
>>>>>>
>>>>>> On Thu, Oct 18, 2012 at 7:32 PM, Kenneth Zadeck
>>>>>> <zadeck@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).
>>>
>>> This reply applies just as much to this patch as patch 6.
>>> I morally agree with you 100%.    But the code does not agree with you.
>>>
>>> On patch 6, there are about 450 places where we want to take the lower
>>> hwi
>>> worth of bits out of a int cst.    Of those, only 5 places use the
>>> function
>>> that allows the signedness to be passed in.   The rest make the
>>> signedness
>>> decision based on the local code and completely ignore any information in
>>> the type.    Of those 5 that do allow the signedness to be passed in,
>>> only
>>> three of them actually pass it in based on the signedness of the variable
>>> they are accessing.
>>>
>>> I am sure that a lot of these are wrong.  But i could not tell you which
>>> are
>>> and which are not.
>>> luckily, a lot of this will go away with the full wide-int code because i
>>> just do most of this math in the full precision so the issue never comes
>>> up.
>>> But after i am finished, there will still be a fair number of places that
>>> do
>>> this.   (luckily, a large number of them are pulling the number out and
>>> comparing it to the precision of something, so this is likely to be
>>> harmless
>>> no matter how the code is written).
>>>
>>> But to a large extent, you are shooting the messenger here, and not
>>> person
>>> who committed the crime.   I will be happy to add some comments to point
>>> the
>>> clients of these to the one that looks at the type.   In looking over the
>>> patch, the only obvious ones that could be changed are the ones in
>>> tree-ssa-uninit.c and the tree-vrp.c. The one in tree-vrp.c just looks
>>> like
>>> that the person writing the code did not know about tree_int_cst_lt and
>>> wrote the check out our himself.  (i will fix this in the tree-vrp patch
>>> that i am working on now. The one in tree-ssa-uniunit looks correct.
>>>
>>> But beyond that, the rest are in the front ends and so i think that this
>>> as
>>> good as you get out of me.
>>
>> Well, if you transform bogus (by moral standards) code into other
>> bogus code the whole point of your patch is to exchange the names
>> of a set of macros / functions to another set of macros / functions.
>>
>> I see no point in that then.
>>
>> Leave broken code as-is.  The more often you touch broken code
>> and just mangle it in some way the harder it gets to get to the
>> point that would maybe reveal the real intent of the original code.
>>
>> Sorry for the harsh words, but to take the example of
>> INT_CST_LT and INT_CST_LT_UNSIGNED -- those are remanents of
>> a world before double_ints existed.  All uses should have been
>> replaced by double_int usage by now; replacing them with something
>> tree-ish is the wrong way.  It might be the correct way if the tree
>> operands have the correct signedness - in which case we already
>> have tree_int_cst_lt for the task.  tree_int_cst_lt[us] is a perversion
>> (that is, wrong when viewed in isolation)!
>
> i do not know if this code is bogus.    i am not a front end person.
> i just need the macros to go away.     And the conventions of the gcc say
> that we do not put real functions with upper case names.
>
> i can of course, convert these to use wide-int and then use the
> wide_int::lts_p and ::ltu_p but that is just going to sweep the issue under
> the rug.   It does not get rid of the issue that in your mind these
> comparisons should be using the signedness of the constants them selves.

But it doesn't end up introducing bogus tree_int_cst_lt[us].  That's the
whole point.  Thus, using wide_int::lt[us]_p is fine with me (or
double_int::lt[us]_p
in the mean time if you care to get rid of the macros now).

Richard.

> Kenny
>
>> Richard.
>>
>>> Kenny
>>>
>>>
>>>>> 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
>>>>>
>>>>>
>
diff mbox

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 930d5e5..5f235fb 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3902,17 +3902,17 @@  shorten_compare (tree *op0_ptr, tree *op1_ptr, tree *restype_ptr,
 
       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);
+	  max_gt = tree_int_cst_lts_p (primop1, maxval);
+	  min_lt = tree_int_cst_lts_p (minval, primop1);
+	  max_lt = tree_int_cst_lts_p (maxval, primop1);
 	}
 
       val = 0;
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f888d32..2ee6eca 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6317,8 +6317,8 @@  type_passed_as (tree type)
   else if (targetm.calls.promote_prototypes (type)
 	   && INTEGRAL_TYPE_P (type)
 	   && COMPLETE_TYPE_P (type)
-	   && INT_CST_LT_UNSIGNED (TYPE_SIZE (type),
-				   TYPE_SIZE (integer_type_node)))
+	   && tree_int_cst_ltu_p (TYPE_SIZE (type),
+				  TYPE_SIZE (integer_type_node)))
     type = integer_type_node;
 
   return type;
@@ -6358,8 +6358,8 @@  convert_for_arg_passing (tree type, tree val, tsubst_flags_t complain)
   else if (targetm.calls.promote_prototypes (type)
 	   && INTEGRAL_TYPE_P (type)
 	   && COMPLETE_TYPE_P (type)
-	   && INT_CST_LT_UNSIGNED (TYPE_SIZE (type),
-				   TYPE_SIZE (integer_type_node)))
+	   && tree_int_cst_ltu_p (TYPE_SIZE (type),
+				  TYPE_SIZE (integer_type_node)))
     val = cp_perform_integral_promotions (val, complain);
   if ((complain & tf_warning)
       && warn_suggest_attribute_format)
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index c62a33e..8183e85 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -3427,7 +3427,7 @@  walk_subobject_offsets (tree type,
 
   /* If this OFFSET is bigger than the MAX_OFFSET, then we should
      stop.  */
-  if (max_offset && INT_CST_LT (max_offset, offset))
+  if (max_offset && tree_int_cst_lts_p (max_offset, offset))
     return 0;
 
   if (type == error_mark_node)
@@ -3582,8 +3582,8 @@  walk_subobject_offsets (tree type,
       for (index = size_zero_node;
 	   /* G++ 3.2 had an off-by-one error here.  */
 	   (abi_version_at_least (2)
-	    ? !INT_CST_LT (TYPE_MAX_VALUE (domain), index)
-	    : INT_CST_LT (index, TYPE_MAX_VALUE (domain)));
+	    ? !tree_int_cst_lts_p (TYPE_MAX_VALUE (domain), index)
+	    : tree_int_cst_lts_p (index, TYPE_MAX_VALUE (domain)));
 	   index = size_binop (PLUS_EXPR, index, size_one_node))
 	{
 	  r = walk_subobject_offsets (TREE_TYPE (type),
@@ -3599,7 +3599,7 @@  walk_subobject_offsets (tree type,
 	  /* If this new OFFSET is bigger than the MAX_OFFSET, then
 	     there's no point in iterating through the remaining
 	     elements of the array.  */
-	  if (max_offset && INT_CST_LT (max_offset, offset))
+	  if (max_offset && tree_int_cst_lts_p (max_offset, offset))
 	    break;
 	}
     }
@@ -5432,7 +5432,7 @@  end_of_class (tree t, int include_virtuals_p)
 	continue;
 
       offset = end_of_base (base_binfo);
-      if (INT_CST_LT_UNSIGNED (result, offset))
+      if (tree_int_cst_ltu_p (result, offset))
 	result = offset;
     }
 
@@ -5442,7 +5442,7 @@  end_of_class (tree t, int include_virtuals_p)
 	 VEC_iterate (tree, vbases, i, base_binfo); i++)
       {
 	offset = end_of_base (base_binfo);
-	if (INT_CST_LT_UNSIGNED (result, offset))
+	if (tree_int_cst_ltu_p (result, offset))
 	  result = offset;
       }
 
@@ -5522,7 +5522,7 @@  include_empty_classes (record_layout_info rli)
 		      CLASSTYPE_AS_BASE (rli->t) != NULL_TREE);
   rli_size = rli_size_unit_so_far (rli);
   if (TREE_CODE (rli_size) == INTEGER_CST
-      && INT_CST_LT_UNSIGNED (rli_size, eoc))
+      && tree_int_cst_ltu_p (rli_size, eoc))
     {
       if (!abi_version_at_least (2))
 	/* In version 1 of the ABI, the size of a class that ends with
@@ -5638,7 +5638,7 @@  layout_class_type (tree t, tree *virtuals_p)
 	 type, then there are some special rules for allocating
 	 it.  */
       if (DECL_C_BIT_FIELD (field)
-	  && INT_CST_LT (TYPE_SIZE (type), DECL_SIZE (field)))
+	  && tree_int_cst_lts_p (TYPE_SIZE (type), DECL_SIZE (field)))
 	{
 	  unsigned int itk;
 	  tree integer_type;
@@ -5649,10 +5649,10 @@  layout_class_type (tree t, tree *virtuals_p)
 	     bits as additional padding.  */
 	  for (itk = itk_char; itk != itk_none; ++itk)
 	    if (integer_types[itk] != NULL_TREE
-		&& (INT_CST_LT (size_int (MAX_FIXED_MODE_SIZE),
-				TYPE_SIZE (integer_types[itk]))
-		    || INT_CST_LT (DECL_SIZE (field),
-				   TYPE_SIZE (integer_types[itk]))))
+		&& (tree_int_cst_lts_p (size_int (MAX_FIXED_MODE_SIZE),
+					TYPE_SIZE (integer_types[itk]))
+		    || tree_int_cst_lts_p (DECL_SIZE (field),
+					   TYPE_SIZE (integer_types[itk]))))
 	      break;
 
 	  /* ITK now indicates a type that is too large for the
@@ -5668,7 +5668,7 @@  layout_class_type (tree t, tree *virtuals_p)
 	     3.2 always created a padding field, even if it had zero
 	     width.  */
 	  if (!abi_version_at_least (2)
-	      || INT_CST_LT (TYPE_SIZE (integer_type), DECL_SIZE (field)))
+	      || tree_int_cst_lts_p (TYPE_SIZE (integer_type), DECL_SIZE (field)))
 	    {
 	      if (abi_version_at_least (2) && TREE_CODE (t) == UNION_TYPE)
 		/* In a union, the padding field must have the full width
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 368d738..ce852ac 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -8075,7 +8075,7 @@  compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
       constant_expression_error (size);
 
       /* An array must have a positive number of elements.  */
-      if (INT_CST_LT (size, integer_zero_node))
+      if (tree_int_cst_lts_p (size, integer_zero_node))
 	{
 	  if (!(complain & tf_error))
 	    return error_mark_node;
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 03eeac2..20fd53c 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -16118,10 +16118,8 @@  fold_relational_const (enum tree_code code, tree type, tree op0, tree op1)
     {
       if (code == EQ_EXPR)
 	result = tree_int_cst_equal (op0, op1);
-      else if (TYPE_UNSIGNED (TREE_TYPE (op0)))
-	result = INT_CST_LT_UNSIGNED (op0, op1);
-      else
-	result = INT_CST_LT (op0, op1);
+      else 
+	result = tree_int_cst_lt (op0, op1);
     }
   else
     return NULL_TREE;
diff --git a/gcc/java/expr.c b/gcc/java/expr.c
index 055d3a6..8f8c928 100644
--- a/gcc/java/expr.c
+++ b/gcc/java/expr.c
@@ -1716,7 +1716,7 @@  build_field_ref (tree self_value, tree self_class, tree name)
 	  tree field_offset = byte_position (field_decl);
 	  if (! page_size)
 	    page_size = size_int (4096); 	      
-	  check = ! INT_CST_LT_UNSIGNED (field_offset, page_size);
+	  check = ! tree_int_cst_ltu_p (field_offset, page_size);
 	}
 
       if (base_type != TREE_TYPE (self_value))
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 0d7f3e4..8837bda 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -290,7 +290,7 @@  default_cxx_get_cookie_size (tree type)
 
   sizetype_size = size_in_bytes (sizetype);
   type_align = size_int (TYPE_ALIGN_UNIT (type));
-  if (INT_CST_LT_UNSIGNED (type_align, sizetype_size))
+  if (tree_int_cst_ltu_p (type_align, sizetype_size))
     cookie_size = sizetype_size;
   else
     cookie_size = type_align;
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index cc58870..14df6e2 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -699,12 +699,12 @@  is_value_included_in (tree val, tree boundary, enum tree_code cmpc)
       if (cmpc == EQ_EXPR)
         result = tree_int_cst_equal (val, boundary);
       else if (cmpc == LT_EXPR)
-        result = INT_CST_LT_UNSIGNED (val, boundary);
+        result = tree_int_cst_ltu_p (val, boundary);
       else
         {
           gcc_assert (cmpc == LE_EXPR);
           result = (tree_int_cst_equal (val, boundary)
-                    || INT_CST_LT_UNSIGNED (val, boundary));
+                    || tree_int_cst_ltu_p (val, boundary));
         }
     }
   else
@@ -712,12 +712,12 @@  is_value_included_in (tree val, tree boundary, enum tree_code cmpc)
       if (cmpc == EQ_EXPR)
         result = tree_int_cst_equal (val, boundary);
       else if (cmpc == LT_EXPR)
-        result = INT_CST_LT (val, boundary);
+        result = tree_int_cst_lts_p (val, boundary);
       else
         {
           gcc_assert (cmpc == LE_EXPR);
           result = (tree_int_cst_equal (val, boundary)
-                    || INT_CST_LT (val, boundary));
+                    || tree_int_cst_lts_p (val, boundary));
         }
     }
 
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index b75d85a..9003ac5 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -1117,10 +1117,10 @@  operand_less_p (tree val, tree val2)
   if (TREE_CODE (val) == INTEGER_CST && TREE_CODE (val2) == INTEGER_CST)
     {
       if (TYPE_UNSIGNED (TREE_TYPE (val)))
-	return INT_CST_LT_UNSIGNED (val, val2);
+	return tree_int_cst_ltu_p (val, val2);
       else
 	{
-	  if (INT_CST_LT (val, val2))
+	  if (tree_int_cst_lts_p (val, val2))
 	    return 1;
 	}
     }
diff --git a/gcc/tree.c b/gcc/tree.c
index d85f9a6..84f2cbc 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -1085,6 +1085,31 @@  wide_int_to_tree (tree type, const wide_int &cst)
   return build_int_cst_wide (type, v.elt (0), v.elt (1));
 }
 
+/* Return 0 or -1 depending on the sign of the cst.  */ 
+
+HOST_WIDE_INT
+int_cst_sign_mask (const_tree op0, int prec)
+{
+  int rec = prec / HOST_BITS_PER_WIDE_INT;
+
+  prec = prec % HOST_BITS_PER_WIDE_INT;
+
+#ifdef NEW_REP_FOR_INT_CST
+  if (rec > TREE_INT_CST_LEN (op0) - 1)
+    rec = TREE_INT_CST_LEN (op0) - 1;
+
+  return (TREE_INT_CST_ELT (op0, rec) << (HOST_BITS_PER_WIDE_INT - prec))
+    >> (HOST_BITS_PER_WIDE_INT - 1);
+#else
+  if (rec)
+    return ((HOST_WIDE_INT)TREE_INT_CST_HIGH (op0) << (HOST_BITS_PER_WIDE_INT - prec))
+      >> (HOST_BITS_PER_WIDE_INT - 1);
+  else
+    return ((HOST_WIDE_INT)TREE_INT_CST_LOW (op0) << (HOST_BITS_PER_WIDE_INT - prec))
+      >> (HOST_BITS_PER_WIDE_INT - 1);
+#endif
+}
+
 /* Returns true if CST fits into range of TYPE.  Signedness of CST is assumed
    to be the same as the signedness of TYPE.  */
 
@@ -1100,6 +1125,102 @@  double_int_fits_to_tree_p (const_tree type, double_int cst)
   return cst == ext;
 }
 
+/* Return true if OP0 < OP1 using signed comparisons.  */
+
+bool
+tree_int_cst_lts_p (const_tree op0, const_tree op1)
+{
+#ifdef NEW_REP_FOR_INT_CST
+  int l0 = TREE_INT_CST_NUNITS (op0) - 1;
+  int l1 = TREE_INT_CST_NUNITS (op1) - 1;
+  int prec = TYPE_PRECISION (TREE_TYPE (op0));
+
+  if (op0 == op1)
+    return false;
+
+  if (prec <= HOST_BITS_PER_WIDE_INT)
+    /* The values are already logically sign extended.  */
+    return TREE_INT_CST_ELT (op0, 0) < TREE_INT_CST_ELT (op1, 0);
+
+  while (l0 > l1)
+    if (TREE_INT_CST_ELT (op0, l0--) < int_cst_sign_mask (op1, prec))
+      return true;
+
+  while (l1 > l0)
+    if (int_cst_sign_mask (op0, prec) < TREE_INT_CST_ELT (op1, l1--))
+      return true;
+
+  while (l0 >= 0)
+    if (TREE_INT_CST_ELT (op0, l0--) < TREE_INT_CST_ELT (op1, l1--))
+      return true;
+
+  return false;
+#else
+  return (TREE_INT_CST_HIGH (op0) < TREE_INT_CST_HIGH (op1)
+	  || (TREE_INT_CST_HIGH (op0) == TREE_INT_CST_HIGH (op1)
+	      && TREE_INT_CST_LOW (op0) < TREE_INT_CST_LOW (op1)));
+#endif
+}
+
+
+/* Return true if OP0 < OP1 using unsigned comparisons.  */
+
+bool
+tree_int_cst_ltu_p (const_tree op0, const_tree op1)
+{
+#ifdef NEW_REP_FOR_INT_CST
+  unsigned HOST_WIDE_INT x0;
+  unsigned HOST_WIDE_INT x1;
+  int l0 = TREE_INT_CST_NUNITS (op0) - 1;
+  int l1 = TREE_INT_CST_NUNITS (op1) - 1;
+  int prec = TYPE_PRECISION (TREE_TYPE (op0));
+
+  if (op0 == op1)
+    return false;
+
+  if (prec < HOST_BITS_PER_WIDE_INT)
+    {
+      x0 = wide_int::zext (TREE_INT_CST_ELT (op0, 0), prec);
+      x1 = wide_int::zext (TREE_INT_CST_ELT (op1, 0), prec);
+
+      return x0 < x1;
+    }
+
+  while (l0 > l1)
+    {
+      x0 = TREE_INT_CST_ELT (op0, l0--);
+      x1 = int_cst_sign_mask (op1, prec);
+      if (x0 < x1)
+	return true;
+    }
+
+  while (l1 > l0)
+    {
+      x0 = int_cst_sign_mask (op0, prec);
+      x1 = TREE_INT_CST_ELT (op1, l1--);
+      if (x0 < x1)
+	return true;
+    }
+
+  while (l0 >= 0)
+    {
+      x0 = TREE_INT_CST_ELT (op0, l0--);
+      x1 = TREE_INT_CST_ELT (op1, l1--);
+      if (x0 < x1)
+	return true;
+    }
+
+  return false;
+#else
+  return (((unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (op0)
+	   < (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (op1))
+	  || (((unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (op0)
+	       == (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (op1))
+	      && TREE_INT_CST_LOW (op0) < TREE_INT_CST_LOW (op1)));
+#endif
+}
+
+
 /* We force the double_int CST to the range of the type TYPE by sign or
    zero extending it.  OVERFLOWABLE indicates if we are interested in
    overflow of the value, when >0 we are only interested in signed
@@ -6513,9 +6634,9 @@  tree_int_cst_lt (const_tree t1, const_tree t2)
 	 type.  */
     }
   else if (!TYPE_UNSIGNED (TREE_TYPE (t1)))
-    return INT_CST_LT (t1, t2);
+    return tree_int_cst_lts_p (t1, t2);
 
-  return INT_CST_LT_UNSIGNED (t1, t2);
+  return tree_int_cst_ltu_p (t1, t2);
 }
 
 /* Returns -1 if T1 < T2, 0 if T1 == T2, and 1 if T1 > T2.  */
diff --git a/gcc/tree.h b/gcc/tree.h
index c9c5869..f5497cd 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -1402,18 +1402,6 @@  extern void omp_clause_range_check_failed (const_tree, const char *, int,
 #define TREE_INT_CST_LOW(NODE) (TREE_INT_CST (NODE).low)
 #define TREE_INT_CST_HIGH(NODE) (TREE_INT_CST (NODE).high)
 
-#define INT_CST_LT(A, B)				\
-  (TREE_INT_CST_HIGH (A) < TREE_INT_CST_HIGH (B)	\
-   || (TREE_INT_CST_HIGH (A) == TREE_INT_CST_HIGH (B)	\
-       && TREE_INT_CST_LOW (A) < TREE_INT_CST_LOW (B)))
-
-#define INT_CST_LT_UNSIGNED(A, B)				\
-  (((unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (A)		\
-    < (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (B))		\
-   || (((unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (A)		\
-	== (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (B))	\
-       && TREE_INT_CST_LOW (A) < TREE_INT_CST_LOW (B)))
-
 struct GTY(()) tree_int_cst {
   struct tree_typed typed;
   double_int int_cst;
@@ -5689,6 +5677,9 @@  extern const_tree strip_invariant_refs (const_tree);
 extern tree lhd_gcc_personality (void);
 extern void assign_assembler_name_if_neeeded (tree);
 extern void warn_deprecated_use (tree, tree);
+extern HOST_WIDE_INT int_cst_sign_mask (const_tree, int);
+extern bool tree_int_cst_lts_p (const_tree, const_tree);
+extern bool tree_int_cst_ltu_p (const_tree, const_tree);
 
 
 /* In cgraph.c */