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
