Re: [range-ops] patch 01/04: types for VR_UNDEFINED and VR_VARYING

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Jul 3, 2019 at 11:19 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 7/3/19 4:28 AM, Richard Biener wrote:
> > On Mon, Jul 1, 2019 at 10:52 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> As discussed before, this enforces types on undefined and varying, which
> >> makes everything more regular, and removes some special casing
> >> throughout range handling.
> >
> > I don't like it too much given you need to introduce that "cache".
> >
> > Why do VARYING or UNDEFINED need a type?  Nobody should ever need
> > to ask for the type of a range anyhow - the type should be always that from
> > the context we're looking at (the SSA name the range is associated with,
> > the operation we are performing on one or two ranges, etc.).
> >
> > Thinking again about this it looks fundamentally wrong to associate
> > a type with the VARYING or UNDEFINED lattice states.
>
> We discussed this 2 weeks ago, and it was my understanding that we had
> an reached an agreement on the general approach.  Types on varying and
> undefined was the *first* thing I brought up.  Explanation quoted below.

Yes, and I asked how you handled that const static node for VARYING
which you answered, well - we could do some caching per type and I
replied I didn't like that very much.

So - I see why it might be "convenient" but I still see no actual
technical requirement to have a type for them.  I see you have
canonicalization for symbolic ranges to integer ranges so
you can have one for varying/undefined to integer ranges as well;
just make that canonicalization take a type argument.

> By the way, the type for varying/undefined requires no space in the
> value_range_base structure, as it is kept in the min/max fields which we
> already use for VR_RANGE/VR_ANTI_RANGE.

You can as well populate those with actual canonical integer range values
then.  [MIN, MAX] and [MAX, MIN] (or whatever we'd consider canonical for
the empty range).

But as said, point me to the place where you need the type of VARYING.
It should already exist since the current code does away without.

I refuse to uglify the current VRP with a not needed type-indexed cache
for VARYING nodes just to make ranger intergation more happy.   Just
ignore that extra 'type' argument in the ranger API then?

Richard.

> Aldy
>
> https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01292.html
>
> In order to unify the APIs for value_range and irange, we'd like to make
> some minor changes to value_range.  We believe most of these changes
> could go in now, and would prefer so, to get broader testing and
> minimize the plethora of changes we drag around on our branch.
>
> First, introduce a type for VR_VARYING and VR_UNDEFINED.
> --------------------------------------------------------
>
> irange utilizes 0 or more sub-ranges to represent a range, and VARYING
> is simply one subrange [MIN, MAX].    value_range represents this with
> VR_VARYING, and since there is no type associated with it, we cannot
> calculate the lower and upper bounds for the range.  There is also a
> lack of canonicalness in value range in that VR_VARYING and [MIN, MAX]
> are two different representations of the same value.
>
> We tried to adjust irange to not associate a type with the empty range
> [] (representing undefined), but found we were unable to perform all
> operations properly.  In particular, we cannot invert an empty range.
> i.e. invert ( [] ) should produce [MIN, MAX].  Again, we need to have a
> type associated with this empty range.
>
> We'd like to tweak value_range so that set_varying() and set_undefined()
> both take a type, and then always set the min/max fields based on that
> type.  This takes no additional memory in the structure, and is
> virtually transparent to all the existing uses of value_range.
>
> This allows:
>      1)  invert to be implemented properly for both VARYING and UNDEFINED
> by simply changing one to the other.
>      2)  the type() method to always work without any special casing by
> simply returning TREE_TYPE(min)
>      3)  the new incoming bounds() routines to work trivially for these
> cases as well (lbound/ubound, num_pairs(), etc).