On 29/07/2022 08:06, Richard Biener via Gcc-patches wrote:
On Thu, Jul 28, 2022 at 6:46 PM Richard Earnshaw
<richard.earns...@foss.arm.com> wrote:
[resend with correct subject line]
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.
I can't comment on the stripping on SUBREGs and friends but it seems
to be conservative apart from
+ if (!flag_strict_aliasing || !MEM_P (dest))
+ return true;
where if dest is not a MEM but were to contain one we'd miss it.
Double-checking
from more RTL literate people appreciated.
There are very few things that can wrap a MEM in a SET_DEST. I'm pretty
sure that's all of them. It certainly matches the code in
cselib_invalidate_rtx which has to deal with this sort of case.
+ /* Lookup the equivalents to the dest. This is more likely to succeed
+ than looking up the equivalents to the source (for example, when the
+ src is some form of constant). */
I think the comment is misleading - we _do_ have to lookup the MEM,
looking up equivalences of a reg or an expression on the RHS isn't
what we are interested in.
OK, I'll try to reword it.
+ return alias_sets_conflict_p (MEM_ALIAS_SET (dest),
+ MEM_ALIAS_SET (src_equiv));
that's not conservative enough - dse.cc has correct boilerplate, we have
to check both MEM_ALIAS_SET and MEM_EXPR here (the latter only
if the former load/store has a MEM_EXPR). Note in particular
using alias_set_subset_of instead of alias_sets_conflict_p.
/* We can only remove the later store if the earlier aliases
at least all accesses the later one. */
&& ((MEM_ALIAS_SET (mem) == MEM_ALIAS_SET (s_info->mem)
|| alias_set_subset_of (MEM_ALIAS_SET (mem),
MEM_ALIAS_SET (s_info->mem)))
&& (!MEM_EXPR (s_info->mem)
|| refs_same_for_tbaa_p (MEM_EXPR (s_info->mem),
MEM_EXPR (mem)))))
OK, that's an easy enough change.
+ /* We failed to find a recorded value in the cselib history, so try the
+ source of this set. */
+ rtx src = SET_SRC (set);
+ while (GET_CODE (src) == SUBREG)
+ src = XEXP (src, 0);
+
+ if (MEM_P (src) && rtx_equal_for_cselib_1 (dest_addr, XEXP (src, 0),
+ GET_MODE (dest), 0))
+ return alias_sets_conflict_p (MEM_ALIAS_SET (dest),
+ MEM_ALIAS_SET (src));
this looks like an odd case to me - wouldn't that only catch things
like self-assignments, aka *p = *p? So I'd simply drop this fallback.
It catches the case of *p = *q when p and q have the same value. It did
come up in testing on x86 (when previously I was aborting to make sure
I'd caught everything). We could leave it out as the fallback case in
this instance is to record a conflict, but it's not a path that's likely
to be performance critical and the probability of this being a redundant
store is quite high. I'll update the comment to make this clearer.
R.
Otherwise it looks OK to me.
Thanks,
Richard.
gcc/ChangeLog:
* 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.
