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.
Richard.
