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

[Jeff Law <jeffreyalaw@gmail.com>]

On 8/2/2022 10:06 AM, Richard Earnshaw wrote:
>
>
> 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.
Understood.  Let's just go with the patch as-is.  That's normal for 
cases where we can't produce a reasonable test.

jeff