From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
To: Jeff Law <jeffreyalaw@gmail.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH v2] cselib: add function to check if SET is redundant [PR106187]
Date: Wed, 3 Aug 2022 10:09:14 +0100 [thread overview]
Message-ID: <d66fe5d3-0322-1841-68c5-dfbebce6a303@foss.arm.com> (raw)
In-Reply-To: <7a7a8864-49d9-b0f0-604b-ec5d98466200@gmail.com>
On 03/08/2022 00:36, Jeff Law wrote:
>
>
> 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.
>
Thanks, committed to trunk. Will work on backports if it doesn't throw
up any issues in the next few days.
R.
> jeff
prev parent reply other threads:[~2022-08-03 9:09 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-29 13:52 Richard Earnshaw
2022-07-30 19:57 ` Jeff Law
2022-08-01 10:38 ` Richard Earnshaw
2022-08-02 16:06 ` Richard Earnshaw
2022-08-02 23:36 ` Jeff Law
2022-08-03 9:09 ` Richard Earnshaw [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d66fe5d3-0322-1841-68c5-dfbebce6a303@foss.arm.com \
--to=richard.earnshaw@foss.arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).