On 09/04/2018 07:42 AM, Richard Biener wrote:
On Thu, Aug 30, 2018 at 9:39 AM Aldy Hernandez <al...@redhat.com> wrote:
On 08/29/2018 12:32 PM, David Malcolm wrote:
On Wed, 2018-08-29 at 06:54 -0400, Aldy Hernandez wrote:
Never say never. Just when I thought I was done...
It looks like I need the special casing we do for pointer types in
extract_range_from_binary_expr_1.
The code is simple enough that we could just duplicate it anywhere
we
need it, but I hate code duplication and keeping track of multiple
versions of the same thing.
No change in functionality.
Tested on x86-64 Linux with all languages.
OK?
A couple of nits I spotted:
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index f20730a85ba..228f20b5203 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -1275,6 +1275,32 @@ set_value_range_with_overflow (value_range &vr,
}
}
+/* Value range wrapper for wide_int_range_pointer. */
+
+static void
+vrp_range_pointer (value_range *vr,
+ enum tree_code code, tree type,
+ value_range *vr0, value_range *vr1)
Looking at the signature of the function, I wondered what the source
and destination of the information is...
vr being the destination and vr0/vr1 being the sources are standard
operating procedure within tree-vrp.c. All the functions are basically
that, that's why I haven't bothered.
Could vr0 and vr1 be const?
...which would require extract_range_into_wide_ints to take a const
value_range *
Yes, but that would require changing all of tree-vrp.c to take const
value_range's. For instance, range_int_cst_p and a slew of other
functions throughout.
... which would require range_int_cst_p to take a const value_range *
(I *think* that's where the yak-shaving would end)
+{
+ signop sign = TYPE_SIGN (type);
+ unsigned prec = TYPE_PRECISION (type);
+ wide_int vr0_min, vr0_max;
+ wide_int vr1_min, vr1_max;
+
+ extract_range_into_wide_ints (vr0, sign, prec, vr0_min, vr0_max);
+ extract_range_into_wide_ints (vr1, sign, prec, vr1_min, vr1_max);
+ wide_int_range_nullness n;
+ n = wide_int_range_pointer (code, sign, vr0_min, vr0_max, vr1_min,
vr1_max);
+ if (n == WIDE_INT_RANGE_UNKNOWN)
+ set_value_range_to_varying (vr);
+ else if (n == WIDE_INT_RANGE_NULL)
+ set_value_range_to_null (vr, type);
+ else if (n == WIDE_INT_RANGE_NONNULL)
+ set_value_range_to_nonnull (vr, type);
+ else
+ gcc_unreachable ();
+}
+
Would it be better to use a "switch (n)" here, rather than a series of
"if"/"else if" for each enum value?
I prefer ifs for a small amount of options, but having the compiler warn
on missing enum alternatives is nice, so I've changed it. Notice
there's more code now though :-(.
I don't like the names *_range_pointer, please change them to sth more
specific like *_range_pointer_binary_op.
Sure.
What's the advantage of "hiding" the resulting ranges behind the
wide_int_range_nullness enum rather than having regular range output?
Our upcoming work has another representation for non-nulls in particular
([-MIN,-1][1,+MAX]), since we don't have anti ranges. I want to share
whatever VRP is currently doing for pointers, without having to depend
on the internals of vrp (value_range *).
What's the value in separating pointer operations at all in the
wide-int interfaces? IIRC the difference is that whenever unioning
is required that when it's a pointer result we should possibly
preserve non-nullness. It's of course also doing less work overall.
I don't follow. What are you suggesting?
So - in the end I'm not convinced that adding this kind of interface
to the wide_int_ variants is worth it and I'd rather keep the existing
VRP code?
Again, I don't want to depend on vr_values or VRP in general.
Aldy
