Richard Biener <rguent...@suse.de> 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 <rguent...@suse.de> > > 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;