Re: [PATCH v2] cselib: add function to check if SET is redundant [PR106187]

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 01/08/2022 11:38, Richard Earnshaw via Gcc-patches wrote:
> 
> 
> On 30/07/2022 20:57, Jeff Law via Gcc-patches wrote:
>>
>>
>> On 7/29/2022 7:52 AM, Richard Earnshaw via Gcc-patches wrote:
>>> A SET operation that writes memory may have the same value as an 
>>> earlier store but if the alias sets of the new and earlier store do 
>>> not conflict then the set is not truly redundant.  This can happen, 
>>> for example, if objects of different types share a stack slot.
>>>
>>> To fix this we define a new function in cselib that first checks for
>>> equality and if that is successful then finds the earlier store in the
>>> value history and checks the alias sets.
>>>
>>> The routine is used in two places elsewhere in the compiler. Firstly
>>> in cfgcleanup and secondly in postreload.
>>>
>>> gcc/ChangeLog:
>>>     * alias.h (mems_same_for_tbaa_p): Declare.
>>>     * alias.cc (mems_same_for_tbaa_p): New function.
>>>     * dse.cc (record_store): Use it instead of open-coding
>>>     alias check.
>>>     * cselib.h (cselib_redundant_set_p): Declare.
>>>     * cselib.cc: Include alias.h
>>>     (cselib_redundant_set_p): New function.
>>>     * cfgcleanup.cc: (mark_effect): Use cselib_redundant_set_p instead
>>>     of rtx_equal_for_cselib_p.
>>>     * postreload.c (reload_cse_simplify): Use cselib_redundant_set_p.
>>>     (reload_cse_noop_set_p): Delete.
>> Seems quite reasonable.   The only question I would have would be 
>> whether or not you considered including the aliasing info into the 
>> hashing used by cselib.  You'd probably still need the bulk of this 
>> patch as well since we could presumably still get a hash conflict with 
>> two stores of the same value to the same location, but with different 
>> alias sets (it's just much less likely), so perhaps it doesn't really 
>> buy us anything.
> 
> I thought about this, but if the alias set were included in the hash, 
> then surely you'd get every alias set in a different value.  Then you'd 
> miss the cases where the alias sets do conflict even though they are not 
> the same.  Anyway, the values /are/ the same so in some circumstances 
> you might want to know that.
> 
>>
>> Ideally this would include a testcase.  You might be able to turn that 
>> non-executawble reduced case into something useful by scanning the 
>> post-reload dumps.
> 
> I considered this as well, but the testcase I have is far too fragile, I 
> think.  The existing test only fails on Arm, only fails on 11.2 (not 
> 11.3 or gcc-12 onwards), relies on two objects with the same value being 
> in distinct alias sets but still assigned to the same stack slot and for 
> some copy dance to end up trying to write back the original value to the 
> same slot but with a non-conflicting set.  And finally, the scheduler 
> has to then try to move a load past the non-aliasing store.


> 
> To get anywhere close to this I think we'd need something akin to the 
> gimple reader but for RTL so that we could set up all the conditions for 
> the failure without the risk of an earlier transform blowing the test away.

I wasn't aware of the rtl reader already in the compiler.  But it 
doesn't really get me any closer as it is lacking in so many regards:

- It can't handle (const_double:SF ...) - it tries to handle the 
argument as an int.  This is a consequence, I think, of the reader being 
based on that for reading machine descriptions where FP const_double is 
simply never encountered.

- It doesn't seem to handle anything much more than very basic types, 
and in particular appears to have no way of ensuring that alias sets 
match up with the type system.

> 
> I even considered whether we could start with a gimple dump and 
> bypassing all the tree/gimple transformations, but even that would be 
> still at the mercy of the stack-slot allocation algorithm.

I spent a while trying to get some gimple out of the dumpers in a form 
that was usable, but that's pretty much a non-starter.  To make it work 
we'd need to add support for gimple clobbers on objects - without that 
there's no way to get the stack-slot sharing code to work.  Furthermore, 
even feeding fully-optimized gimple directly into expand is such a long 
way from the postreload pass, that I can't believe the testcase would 
remain stable for long.

And the other major issue is that the original testcase is heavily 
templated C++ and neither of the parsers gimple or rtl is supported in 
cc1plus: converting the boilerplate to be C-friendly is probably going 
to be hard.

I can't afford to spend much more time on this, especially given the 
low-quality test we're going to get out of the end of the process.

> 
>>
>> Jeff
> 
> R.

R.