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

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Jul 28, 2022 at 6:46 PM Richard Earnshaw
<Richard.Earnshaw@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.

+  /* 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.

+               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)))))

+  /* 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.

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.