On Fri, 26 Jan 2018, Richard Biener wrote: > On Fri, 26 Jan 2018, Jakub Jelinek wrote: > > > On Thu, Jan 25, 2018 at 12:18:21PM +0100, Richard Biener wrote: > > > 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); > > > > Is alias set 0 the only easily computable choice if there is a mismatch? > > Also, isn't it fine if one of the alias set is a subset of the other one? > > > > More importantly, I think set_mem_alias_set (r_info->mem, 0) can't work, > > r_info->mem is a result of canon_rtx (SET_DEST (body)), so sometimes could > > be the MEM that appears in the instruction, but at other times could be a > > different pointer and changing that wouldn't change anything in the actual > > instruction. So, in order to do this you'd need to add probably another > > field and record the original SET_DEST (body) record_store is called with. > > Uh, indeed. See my mail in response to Richard which comes up with > an alternate patch avoiding this issue. > > > Even that doesn't need to be something that really appears in the > > instruction, but the exception in that case is the memset call and that > > semantically has alias set of 0. > > > > > --- 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; > > > > Is it desirable to modify an existing test, rather than say macroize this > > and add pr77745-2.C that will define some macro and include this pr77745.C, > > such that we cover both noinline and without noinline? > > Yeah, I'll do that.
This is what I have applied, bootstrapped and tested on x86_64-unknown-linux-gnu. Richard. 2018-01-26 Richard Biener <rguent...@suse.de> PR rtl-optimization/84003 * dse.c (record_store): Only record redundant stores when the earlier store aliases at least all accesses the later one does. * g++.dg/torture/pr77745.C: Mark foo noinline to trigger latent bug in DSE if NOINLINE is appropriately defined. * g++.dg/torture/pr77745-2.C: New testcase including pr77745.C and defining NOINLINE. Index: gcc/dse.c =================================================================== --- gcc/dse.c (revision 257077) +++ gcc/dse.c (working copy) @@ -1532,7 +1532,12 @@ record_store (rtx body, bb_info_t bb_inf && known_subrange_p (offset, width, s_info->offset, s_info->width) && all_positions_needed_p (s_info, offset - s_info->offset, - width)) + width) + /* 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)))) { if (GET_MODE (mem) == BLKmode) { Index: gcc/testsuite/g++.dg/torture/pr77745.C =================================================================== --- gcc/testsuite/g++.dg/torture/pr77745.C (revision 257080) +++ gcc/testsuite/g++.dg/torture/pr77745.C (working copy) @@ -1,8 +1,12 @@ // { dg-do run } +#ifndef NOINLINE +#define NOINLINE /* */ +#endif + inline void* operator new(__SIZE_TYPE__, void* __p) noexcept { return __p; } -long foo(char *c1, char *c2) +long NOINLINE foo(char *c1, char *c2) { long *p1 = new (c1) long; *p1 = 100; Index: gcc/testsuite/g++.dg/torture/pr77745-2.C =================================================================== --- gcc/testsuite/g++.dg/torture/pr77745-2.C (nonexistent) +++ gcc/testsuite/g++.dg/torture/pr77745-2.C (working copy) @@ -0,0 +1,4 @@ +// { dg-do run } + +#define NOINLINE __attribute__((noinline)) +#include "pr77745.C"