On Tue, Sep 4, 2018 at 3:51 PM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> On 09/04/2018 09:41 AM, Richard Biener wrote:
> > On Tue, Sep 4, 2018 at 3:21 PM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >>
> >>
> >> On 09/04/2018 08:29 AM, Richard Biener wrote:
> >>> On Tue, Sep 4, 2018 at 2:09 PM Aldy Hernandez <al...@redhat.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> 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?
> >>>
> >>> I'm thinking out loud. Here adding sort-of a "preferencing" to the
> >>> more general handling of the ops would do the same trick, without
> >>> restricting that to "pointers". For example if a pass would be interested
> >>> in knowing whether a divisor is zero it would also rather preserve
> >>> non-nullness and trade that for precision in other parts of the range,
> >>> say, if you had [-1, -1] [1, +INF] then the smallest unioning result
> >>> is [-1, +INF] but ~[0, 0] is also a valid result which preserves
> >>> non-zeroness.
> >>>
> >>> So for wide-int workers I don't believe in tagging sth as pointers.
> >>> Rather
> >>> than that ops might get one of
> >>>
> >>> enum vr_preference { VR_PREF_SMALL, VR_PREF_NONNULL,
> >>> VR_PREF_NONNEGATIVE, ... }
> >>>
> >>> ?
> >>
> >> Neat. I think this is worth exploring, but perhaps something to be
> >> looked at as a further enhancement?
> >>
> >>>
> >>>>>
> >>>>> 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.
> >>>
> >>> Sure. But the general handling of the ops (just treat POINTER_PLUS like
> >>> PLUS)
> >>> should do range operations just fine. It's only the preferencing you'd
> >>> lose
> >>> and that preferencing looks like a more general feature than "lets have
> >>> this
> >>> function dealing with a small subset of binary ops on pointers"?
> >>
> >> Something like MIN/MAX, that is specially handled for binary ops, can't
> >> be treated with the general min/max range operations. Take for instance
> >> MIN(NULL, NON-NULL) which is basically MIN([0], [1..MAX]). Generic
> >> range ops would yield 0/NULL, whereas current VRP returns VARYING.
> >>
> >> Similarly for BIT_AND_EXPR. Imagine [1,5] & [10,20] for pointers.
> >> Handling this generically would yield [0] (NULL), whereas the correct
> >> answer is NON-NULL. And yes, [1,5] and [10,20] can actually happen, as
> >> I've mentioned in another thread. I think it's libgcc that has code
> >> that does:
> >>
> >> if (some_pointer == (pointer_type *) 1)
> >> else if (some_pointer == (pointer_type *) 2)
> >> etc etc.
> >>
> >> Again, I think we could address your preference idea as an enhancement
> >> if you feel strongly about it.
> >
> > Ah, OK. It basically boils down to pointer operations having special
> > semantics
> > (similar to overflow definedness).
>
> Yes.
>
> >
> >> We could make wide_int_range_pointer an inline function, and the penalty
> >> for VRP would only be the conversion to wide ints in vrp_range_pointer
> >> (extract_range_into_wide_ints actually).
> >>
> >>>
> >>> The preferencing above would get passed down to the range unioning code.
> >>>
> >>> Btw, since your wide-int-range code doesn't even have ANTI_RANGE, how
> >>> do you represent non-zero in the API?
> >>
> >> The way god intended of course, with the truth! As I said earlier:
> >>
> >> >> Our upcoming work has another representation for non-nulls in
> >> particular
> >> >> ([-MIN,-1][1,+MAX]), since we don't have anti ranges.
> >>
> >> :-)
> >>
> >>>
> >>> As said, splitting this out in the way of your patch looks "premature",
> >>> there's not enough of the big picture visible for me to say it makes
> >>> sense.
> >>
> >> You could always look at svn/ssa-range :-P. It's been merged with
> >> mainline as of early last week.
> >
> > Eh.
> >
> >> Splitting this out makes my life a lot easier, with virtually no penalty
> >> to VRP. And we could always revisit it later. But if you feel strongly
> >> about it, I could inline the pointer handling into our code, and revisit
> >> this later with you.
> >
> > Yeah, please do so - I'm leaving for the Cauldron soon. Looking at
> > the current wide-int-range.h header doesn't reveal a consistent API :/
>
> Suggestions welcome :).
>
> Just to make sure, was that OK and approval, provided I make the
> function an inline?
It was to "inline the pointer handling into our code"
>
> Thanks, and see you at Cauldron (albeit a few days late).
> Aldy
