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.
What's the advantage of "hiding" the resulting ranges behind the
wide_int_range_nullness enum rather than having regular range output?
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.
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?
Richard.
>
> Aldy
