This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: VRP: abstract out POINTER_TYPE_P handling
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Aldy Hernandez <aldyh at redhat dot com>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 29 Aug 2018 12:32:58 -0400
- Subject: Re: VRP: abstract out POINTER_TYPE_P handling
- References: <5db8448c-3822-35ad-016d-274fcfdc90a5@redhat.com>
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...
Could vr0 and vr1 be const?
...which would require extract_range_into_wide_ints to take a const
value_range *
... 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?
[...snip...]
Hope this is constructive
Dave