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

[Andrew MacLeod <amacleod at redhat dot com>]

On 7/25/19 11:37 PM, Jeff Law wrote:
> On 7/24/19 12:33 PM, Richard Biener wrote:
> > On July 24, 2019 8:18:57 PM GMT+02:00, Jeff Law <law@redhat.com>
> > wrote:
> > > On 7/24/19 11:00 AM, Richard Biener wrote: [ Big snip, ignore
> > > missing reply attributions... ]
> > >
> > > > > it. But I'd claim that if callers are required not to change
> > > > > these ranges, then the callers are fundamentally broken.  I'm
> > > > > not sure what the "sanitization" is really buying you here.
> > > > > Can you point to something specific?
> > > > >
> > > > > > But you lose the sanitizing that nobody can change it and the
> > > > > >   changed info leaks to other SSA vars.
> > > > > >
> > > > > > As said, fix all callers to deal with NULL.
> > > > > >
> > > > > > But I argue the current code is exactly optimal and safe.
> > > > > ANd I'd argue that it's just plain broken and that the
> > > > > sanitization you're referring to points to something broken
> > > > > elsewhere,  higher up in the callers.
> > > > Another option is to make get_value_range return by value and
> > > > the only way to change the lattice to call an appropriate set
> > > > function. I think we already do the latter in all cases (but we
> > > > use get_value_range in the setter) and returning by reference is
> > > > just eliding the copy.
> > > OK, so what I think you're getting at (and please correct me if
> > > I'm wrong) is that once the lattice values are set, you don't want
> > > something changing the recorded ranges underneath?
> > >
> > > ISTM the way to enforce that is to embed the concept in the class
> > > and enforce it by not allowing direct manipulation of range by the
> > > clients. So a client that wants this behavior somehow tells the
> > > class that ranges are "set in stone" and from that point the
> > > setters don't allow changing the underlying ranges.
> > Yes. You'll see that nearly all callers do
> >
> > Value_range vr = *get_value_range (name);
> >
> > Modify
> >
> > Update_value_range (name, &vr) ;
> >
> > And returning by reference was mostly an optimization. We _did_ have
> > callers Changing the range in place and the const varying catched
> > those.
> >
> > When returning by value we can return individual VARYINGs not in the
> > lattice if we decide that's what we want.
> >
> > > I just want to make sure we're on the same page WRT why you think
> > > the constant varying range object is useful.
> > As said it's an optimization. We do not want to reallocate the
> > lattice. And we want lattice updating to happen in a controlled
> > manner, so returning a pointer into the lattice is bad design at this
> > point.
> But I would claim that the current state is dreadful.  Consider that
> when gimple-fold asks for a new SSA_NAME, it could get a recycled one,
> in which case we get a real range.  Or if it doens't get a recycled
> name, then we get the const varying node.  The inconsistently is silly
> when we can just reallocate the underlying object.
>
> Between recycling of SSA_NAMEs and allocating a bit of additional space
> (say rounding up to some reasonable boundary) I'd bet you'd never be
> able to measure the reallocation in practice.
I annotated the patch yesterday to actually log reallocations and ran a 
couple of bootstraps...

If we don't add any extra space in the vector initially (as it is 
allocated today) , it is re-sized a total of 201 times.  Of those, 93 
get back the same pointer so no resize actually happens.

IF we use the patch as I initially propose, where we add 10% to the 
vector to start, we re-size 10 times, all from either 18 to 25 , or 8 to 
14,so very small numbers of ssaname functions, where the 10% doesnt 
really do much.  Those were all re-allocations but one.   The initial 
resize does seem to help prevent any larger reallocations,

THat doesn't seem like that bad of a thing over all, and we could tweak 
the initial size a bit more if it would help? to deal with the small 
cases, we could make it num_names+10%+10 or something even.

I feel it would be safer.. It seems to me the CONST solution cannot be 
disabled.. ie, even a non-checking production compiler would go boom if 
it triggered.

As for addressing the issue that the CONST approach is trying to 
resolve, Could we change all the set/update routines to do something like

gcc_checking_assert (new_range->varying_p ()  || !values_propagated);

that way we'll trigger an assert if we try to change any value to 
something other than varying when values_propagated is set?

Andrew