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

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

On Tue, Jul 9, 2019 at 9:28 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 7/4/19 6:33 AM, Richard Biener wrote:
> > On Wed, Jul 3, 2019 at 2:17 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> On 7/3/19 7:08 AM, Richard Biener wrote:
> >>> On Wed, Jul 3, 2019 at 11:19 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> >> 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.
>
> Basically, everywhere.  By having a type for varying/undefined, we don't
> have to special case anything.  Sure, we could for example, special case
> the invert operation for undefined / varying.  And we could special case
> everything dealing with ranges to handle varying and undefined, but why?
>   We could also pass a type argument everywhere, but that's just ugly.
> However, I do understand your objection to the type cache.
>
> How about the attached approach?  Set the type for varying/undefined
> when we know it, while avoiding touching the CONST varying.  Then right
> before calling the ranger, pass down a new varying node with min/max for
> any varyings that were still typeless until that point.
>
> I have taken care of never adding a set_varying() that was not already
> there.  Would this keep the const happy?
>
> Technically we don't need to set varying/undef types for every instance
> in VRP, but we need it at least for the code that will be shared with
> range-ops (extract_range_from_multiplicative_op, union, intersect, etc).
>   I just figured if we have the information, might as well set it for
> consistency.
>
> If you like this approach, I can rebase the other patches that depend on
> this one.

OK, so I went ant checked what you do for class irange which has
a type but no kind member (but constructors with a kind).  It also
uses wide_int members for storage.  For a pure integer constant
range representation this represents somewhat odd choices;  I'd
have elided the m_type member completely here, it seems fully
redundant.  Only range operations need to be carried out in a
specific type (what I was suggesting above).  Even the precision
encoded in the wide_int members is redundant then (I'd have
expected widest_int here and trailing-wide-ints for optimizing
storage).

Then class range_operator looks a bit strange to me (looking
just at the header).  Ugh, so it is all virtual because
you have one instance per tree code.  What an odd choice.
Why didn't you simply go with passing tree_code  (and type!)
to fold_range/op_range?  The API also seems to be oddly
constrained to binary ops.  Anyway, the way you build
the operator table requires an awful lot of global C++ ctor
invocations, sth we generally try to avoid.  But I'm getting
into too many details here.

So - to answer your question above, I'd like you to pass down
a type to operations.  Because that's what is fundamentally
required - a range doesn't have a "type" and the current
value_range_base doesn't fall into the trap of needing one.

Richard.

>
> Aldy