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

[Andrew MacLeod <amacleod@redhat.com>]

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