On Fri, Jul 26, 2019 at 4:32 PM Andrew MacLeod <amacl...@redhat.com> wrote:
>
> 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 <l...@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?
I think the constness is appropriately addressed by my recent API update
to split the get-value from get-lattice-entry and properly forcing lattice
update to go through the few setters.
I'm not totally against making the lattice expandable, as the followup
patch to the original re-org notices we do run into "new" stmts during
the combined analysis/propagation stages the DOM-based users use.
And yes, allocating the lattice with some initial head-room is of course
the way to go (I'd even just do 2 * num_ssa_names...). Avoiding
"useless" allocations of VR_VARYING lattice entries (where a NULL
does it just fine) is another optimization. But yeah, we do not
"free" lattice entries when they become VR_VARYING and further
return the shared const entry (but we could).
That still leaves me with the objection to making VARYING typed.
IMHO the vr-values.c lattice should look like
enum lattice_val { UNDEFINED, CONST, VARYING };
struct lattice_entry {
lattice_val val;
value_range *range;
};
and we'd have _no_ value_range (NULL) for UNDEFINED and VARYING
in the _lattice_. And CONST (aka classical we have a value) would then
record a value_range.
That we currently mix lattice state and the value-range types
(range, anti-range) is just historical. The CONST lattice state could even
be split further, CONST_SCALAR, CONST_RANGE, VARYING and the
entry be a union of either value_range * or tree.
Richard.
> Andrew
>
