[PATCH] Convert strlen pass from evrp to ranger.

[Jeff Law]

On 10/18/2021 2:17 AM, Aldy Hernandez wrote:
>
>
> On 10/18/21 12:52 AM, Jeff Law wrote:
>>
>>
>> On 10/8/2021 9:12 AM, Aldy Hernandez via Gcc-patches wrote:
>>> The following patch converts the strlen pass from evrp to ranger,
>>> leaving DOM as the last remaining user.
>> So is there any reason why we can't convert DOM as well?   DOM's use 
>> of EVRP is pretty limited.  You've mentioned FP bits before, but my 
>> recollection is those are not part of the EVRP analysis DOM uses. 
>> Hell, give me a little guidance and I'll do the work...
>
> Not only will I take you up on that offer, but I can provide 90% of 
> the work.  Here be dragons, though (well, for me, maybe not for you ;-)).
>
> DOM is actually an evrp pass at -O1 in disguise.  The reason it really 
> is a covert evrp pass is because:
>
> a) It calls extract_range_from_stmt on each statement.
>
> b) It folds conditionals with simplify_using_ranges.
>
> c) But most importantly, it exports discovered ranges when it's done 
> (evrp_range_analyzer(true)).
>
> If you look at the evrp pass, you'll notice that that's basically what 
> it does, albeit with the substitute and fold engine, which also calls 
> gimple fold plus other goodies.
>
> But I could argue that we've made DOM into an evrp pass without 
> noticing.  The last item (c) is particularly invasive because these 
> exported ranges show up in other passes unexpectedly.  For instance, I 
> saw an RTL pass at -O1 miss an optimization because it was dependent 
> on some global range being set.  IMO, DOM should not export global 
> ranges it discovered during its walk (do one thing and do it well), 
> but I leave it to you experts to pontificate.
All true.  But I don't think we've got many, if any, hard dependencies 
on those behaviors.

>
> The attached patch is rather trivial.  It's mostly deleting state.  It 
> seems DOM spends a lot of time massaging the IL so that it can fold 
> conditionals or thread paths.  None of this is needed, because the 
> ranger can do all of this.  Well, except floats, but...
Massaging the IL should only take two forms IIRC.

First, if we have a simplification we can do.  That could be const/copy 
propagation, replacing an expression with an SSA_NAME or constant and 
the like.  It doesn't massage the IL just to massage the IL.

Second, we do temporarily copy propagate the current known values of an 
SSA name into use points and then see if that allows us to determine if 
a statement is already in the hash tables.  But we undo that so that 
nobody should see that temporary change in state.

Finally, it does create some expressions & statements on the fly to 
enter them into the tables.  For example, if it sees a store, it'll 
create a load with the source & dest interchanged and enter that into 
the expression table.  But none of this stuff ever shows up in the IL.  
It's just to create entries in the expression tables.

So ITSM the only real concern would be if those temporary const/copy 
propagations were still in the IL and we called back into Ranger and it 
poked at that data somehow.
>
> That's the good news.  The bad news is that DOM changes the IL as it 
> goes and the patch doesn't bootstrap.  Andrew insists that we should 
> work even with DOM's changing IL, but last time we played this dance 
> with the substitute_and_fold engine, there were some tweaks needed to 
> the ranger.  Could be this, but I haven't investigated.  It could also 
> be that the failures I was seeing were just DOM things that were no 
> longer needed (shuffling the IL to simplify things for evrp).
So if we're referring to those temporary const/copy propagations 
"escaping" into Ranger, then I would fully expect that to cause 
problems.  Essentially they're path sensitive const/copy propagations 
and may not be valid on all the paths through the CFG to the statement 
where the propagation occurs



>
> This just needs a little shepherding from a DOM expert ;-).  If you 
> get it to bootstrap, I could take care of the tests, performance, and 
> making sure we're getting the same number of threads etc.
>
>>
>>>
>>> No additional cleanups have been done.  For example, the strlen pass
>>> still has uses of VR_ANTI_RANGE, and the sprintf still passes around
>>> pairs of integers instead of using a proper range.  Fixing this
>>> could further improve these passes.
>>>
>>> As a further enhancement, if the relevant maintainers deem useful,
>>> the domwalk could be removed from strlen.  That is, unless the pass
>>> needs it for something else.
>> The dom walk was strictly for the benefit of EVRP when it was added.  
>> So I think it can get zapped once the pass is converted.
>
> Jakub mentioned a while ago, that the strlen pass itself needs DOM, so 
> perhaps this needs to stay.
Yes, strlen wants DOM.  But the printf bits don't really need it. Well,  
maybe they do now that the printf warnings are more tightly integrated 
with the strlen bits.


jeff