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 <amacl...@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 <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 > >
