Richard Biener <[email protected]> writes:
> The following patch fixes PR84003 where RTL DSE removes a redundant
> store (a store storing the same value as an earlier store) but in
> doing this changing program semantics because the later store changes
> the effective type of the memory location. This in turn allows
> sched2 to do an originally invalid transform.
>
> The fix is to harden DSE so that while removing the later store
> the earlier stores alias-sets are changed to reflect both dynamic
> types - which means using alias-set zero.
>
> On GIMPLE we simply avoid removing such type-changing stores because
> we cannot easily get hold on the earlier store.
>
> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>
> Disclaimer: this is a "local" fix, I didn't try to understand much
> of the DSE data structures but only learned from +-10 lines around
> the affected transform which I found by searching for the dumped
> messages ... Richard, you contributed the pass so please review.
So the file says. In reality I only wrote an early version, and what
was committed contains very little of that. So TBH this pass is almost
a complete unknown to me. That said...
Might it be worth keeping the store instead? Giving things alias set
zero seems like a pretty big hammer and would turn the remaining store
into something close to a scheduling barrier.
IOW, how about checking alias_set_subset_of in:
/* Even if PTR won't be eliminated as unneeded, if both
PTR and this insn store the same constant value, we might
eliminate this insn instead. */
if (s_info->const_rhs
&& const_rhs
&& known_subrange_p (offset, width,
s_info->offset, s_info->width)
&& all_positions_needed_p (s_info, offset - s_info->offset,
width))
instead?
Failing that:
> 2018-01-25 Richard Biener <[email protected]>
>
> PR rtl-optimization/84003
> * dse.c (dse_step1): When removing redundant stores make sure
> to adjust the earlier stores alias-set to match semantics of
> the removed one and its own.
> (dse_step6): Likewise.
>
> * g++.dg/torture/pr77745.C: Mark foo noinline to trigger
> latent bug in DSE.
>
> Index: gcc/dse.c
> ===================================================================
> --- gcc/dse.c (revision 257043)
> +++ gcc/dse.c (working copy)
> @@ -2725,6 +2725,19 @@ dse_step1 (void)
> "eliminated\n",
> INSN_UID (ptr->insn),
> INSN_UID (s_info->redundant_reason->insn));
> + /* If the later store we delete could have changed the
> + dynamic type of the memory make sure the one we
> + preserve serves as a store for all loads that could
> + validly have accessed the one we delete. */
> + store_info *r_info = s_info->redundant_reason->store_rec;
> + while (r_info)
> + {
> + if (r_info->is_set
> + && (MEM_ALIAS_SET (s_info->mem)
> + != MEM_ALIAS_SET (r_info->mem)))
> + set_mem_alias_set (r_info->mem, 0);
> + r_info = r_info->next;
> + }
> delete_dead_store_insn (ptr);
> }
> free_store_info (ptr);
> @@ -3512,6 +3525,19 @@ dse_step6 (void)
> "eliminated\n",
> INSN_UID (insn_info->insn),
> INSN_UID (rinsn));
> + /* If the later store we delete could have changed the
> + dynamic type of the memory make sure the one we
> + preserve serves as a store for all loads that could
> + validly have accessed the one we delete. */
> + store_info *r_info = s_info->redundant_reason->store_rec;
> + while (r_info)
> + {
> + if (r_info->is_set
> + && (MEM_ALIAS_SET (s_info->mem)
> + != MEM_ALIAS_SET (r_info->mem)))
> + set_mem_alias_set (r_info->mem, 0);
> + r_info = r_info->next;
> + }
I think this is subtle enough to be worth factoring out. How about
checking alias_set_subset_of, rather than checking for equality?
Thanks,
Richard
> delete_dead_store_insn (insn_info);
> }
> }
> Index: gcc/testsuite/g++.dg/torture/pr77745.C
> ===================================================================
> --- gcc/testsuite/g++.dg/torture/pr77745.C (revision 257043)
> +++ gcc/testsuite/g++.dg/torture/pr77745.C (working copy)
> @@ -2,7 +2,7 @@
>
> inline void* operator new(__SIZE_TYPE__, void* __p) noexcept { return __p; }
>
> -long foo(char *c1, char *c2)
> +long __attribute__((noinline)) foo(char *c1, char *c2)
> {
> long *p1 = new (c1) long;
> *p1 = 100;
