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

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 29/07/2022 08:06, Richard Biener via Gcc-patches wrote:
> 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.

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.