Re: [PATCH] tree-optimization/106593 - fix ICE with backward threading

[Aldy Hernandez <aldyh@redhat.com>]

In that case Richi, go right ahead with your original patch. I for one am
happy we can use range_on_entry, which always seemed cleaner.

Aldy

On Fri, Aug 12, 2022, 16:07 Andrew MacLeod <amacleod@redhat.com> wrote:

>
> 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 <rguenther@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
>
>