On 8/12/22 09:38, Andrew MacLeod wrote:
On 8/12/22 07:31, Aldy Hernandez wrote:
On Fri, Aug 12, 2022 at 12:59 PM Richard Biener <rguent...@suse.de>
wrote:
With the last re-org I failed to make sure to not add SSA names
nor supported by ranger into m_imports which then triggers an
ICE in range_on_path_entry because range_of_expr returns false. I've
noticed that range_on_path_entry does mightly complicated things
that don't make sense to me and the commentary might just be
out of date. For the sake of it I replaced it with range_on_entry
and statistics show we thread _more_ jumps with that, so better
not do magic there.
Hang on, hang on. range_on_path_entry was written that way for a
reason. Andrew and I had numerous discussions about this. For that
matter, my first implementation did exactly what you're proposing, but
he had reservations about using range_on_entry, which IIRC he thought
should be removed from the (public) API because it had a tendency to
blow up lookups.
Let's wait for Andrew to chime in on this. If indeed the commentary
is out of date, I would much rather use range_on_entry like you
propose, but he and I have fought many times about this... over
various versions of the path solver :).
The original issue with range-on-entry is one needed to be very
careful with it. If you ask for range-on-entry of something which is
not dominated by the definition, then the cache filling walk was
getting filled all the way back to the top of the IL, and that was
both a waste of time and memory., and in some pathological cases was
outrageous. And it was happening more frequently than one imagines...
even if accidentally. I think the most frequent accidental misuse we
saw was calling range on entry for a def within the block, or a PHI
for the block.
Its a legitimate issue for used before defined cases, but there isnt
much we can do about those anyway,
range_of_expr on any stmt within a block, when the definition comes
from outside he block causes ranger to trigger its internal
range-on-entry "more safely", which is why it didn't need to be part
of the API... but i admit it does cause some conniptions when for
instance there is no stmt in the block.
That said, the improvements since then to the cache to be able to
always use dominators, and selectively update the cache at strategic
locations probably removes most issues with it. That plus we're more
careful about timing things these days to make sure something horrid
isn't introduced. I also notice all my internal range_on_entry and
_exit routines have evolved and are much cleaner than they once were.
So. now that we are sufficiently mature in this space... I think we
can promote range_on_entry and range_on_exit to full public API.. It
does seem that there is some use practical use for them.
Andrew
PS. It might even be worthwhile to add an assert to make sure it isnt
being called on the def block.. just to avoid that particular stupidty
:-) I'll take care of doing this.
Actually, as I look at it, perhaps better to leave things as they are..
ie, not promote it to a part of the range_query API.. that appears
fraught with derived issues in other places.
Continue to leave it in rangers public API and anyone using a ranger can
use it. I will add the assert to make sure its not abused in the common
way of the past.
And yes, this will dramatically simplify the path_entry routine :-)
Andrew
