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.
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
