This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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 <>
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);


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

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
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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]