On Wed, Jul 3, 2019 at 2:17 PM Aldy Hernandez <al...@redhat.com> wrote:
>
> On 7/3/19 7:08 AM, Richard Biener wrote:
> > On Wed, Jul 3, 2019 at 11:19 AM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >>
> >>
> >> On 7/3/19 4:28 AM, Richard Biener wrote:
> >>> On Mon, Jul 1, 2019 at 10:52 AM Aldy Hernandez <al...@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?
>
> Ok, I see. Your main beef is with the type cache.
yes.
> How about we keep VARYING and UNDEFINED typeless until right before we
> call into the ranger. At which point, we have can populate min/max
> because we have the tree_code and the type handy. So right before we
> call into the ranger do:
>
> if (varying_p ())
> foo->set_varying(TYPE);
>
> This would avoid the type cache, and keep the ranger happy.
you cannot do set_varying on the static const range but instead you'd do
value_range tem (*foo);
if (varying_p ())
tem->set_full_range (TYPE);
which I think we already do in some places. Thus my question _where_
you actually need this.
> Another option, as you've hinted, would be to normalize VARYING into
> [MIN, MAX] before calling into the ranger. The problem with this
> approach is that we would then need to change varying_p() to something like:
>
> value_range_base::varying_p()
> {
> return (m_kind == VR_VARYING ||
> (vrp_val_is_min (m_min) && vrp_val_is_max (m_max));
> }
>
> Thus slowing everyone down (remember both range-ops and tree-vrp will
> share the implementation for varying_p). Plus, I'd prefer to keep one
> representation for VARYING, that is m_kind == VR_VARYING.
Yes, I wasn't really suggesting the above but have m_kind == VR_VARYING
and for ranges we want to support [MIN, MAX] on have those pre-populated
when we drop sth to varying. For pointers this range is pointless for example
and the static const could simply leave it unset as well.
> Perhaps this last alternative would be more consistent-- never allowing
> types for VARYING, at the expense of the calls to vrp_val_is*.
It doesn't work for the static const varying though.
Another way to circumvent the issue is to make get_value_range
return NULL if it doesn't have any range in the lattice (thus the
static const varying goes away). Then callers need to deal with this
of course.
Just teach ranger about different kind of ranges?
Richard.
> Thoughts?
>
> Aldy
