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

[Jeff Law <law@redhat.com>]

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.

jeff