On Fri, Oct 19, 2012 at 2:12 PM, Kenneth Zadeck
<zad...@naturalbridge.com> wrote:
>
> On 10/19/2012 07:58 AM, Richard Biener wrote:
>>
>> On Fri, Oct 19, 2012 at 1:49 PM, Kenneth Zadeck
>> <zad...@naturalbridge.com> wrote:
>>>
>>> On 10/19/2012 07:13 AM, Richard Biener wrote:
>>>>
>>>> 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).
>>>
>>> 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
>>>>>
>>>>>
>
