On Thu, 25 Jan 2018, Richard Sandiford wrote:
> Richard Sandiford <richard.sandif...@linaro.org> writes:
> > 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?
>
> ...that is, like gimple. :-) Sorry, I spent so long thinking about this
> that I forgot you'd said that gimple already does the same thing.
Yeah, and it still runs into PR82224 ... which means it is not
conservative enough.
> Richard
>
> > 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?
That's what GIMPLE does (use alias_set_subset_of), but it arguably
doesn't work for unions (ok, there even the equality case doesn't
work...).
I guess I thought it's worth trying sth new and adjust the earlier
store ;) Sth that I can't easily do on GIMPLE but everything seemed
to be in place in RTL DSE.
So, when looking at the above code it seems like there are exactly
two insns we look at? s_info and 'mem' I guess. And s_info is
the earlier one.
So sth like
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)
{
? I can confirm that this patch works as well. Is that what we prefer?
It would be consistent with what we do on GIMPLE currently (irrespective
of still existing bugs in this area...).
Note that with the above instead of dse1 removing the later store
dse2 now decides to remove the earlier one... (which is fine!).
So I'm testing the above now, ok if it succeeds?
Thanks,
Richard.
> > 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?
The whole block is probably worth factoring out (I just duplicated into
some already duplicated code parts...). Will do if we decide to go
with this variant.
Richard.
> > 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;
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
