On Fri, 26 Jan 2018, Richard Sandiford wrote: > Richard Biener <rguent...@suse.de> writes: > > 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. > > Yeah. > > > 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? > > Not sure I can call that one, but it seems safer (especially for > backports). > > > 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? > > LGTM (but I can't approve it).
I can self-approve ;) > I was wondering later... if the problem is that we have: > > I1: store X to Y with alias set S1 > I2: load from Y with alias set S1 > I3: store X to Y with alias set S2 > I4: load from Y with alias set S2 > > and removing I3 allows I4 to be scheduled before I1, what would happen > if I1 and I3 store different values (with no dse)? Would something stop > the scheduler from ordering it as: > > I1 I3 I2 I4 > > where I2 would load the wrong value? If so, why didn't that same thing > work here? The scheduler cannot use TBAA when moving a load down across a store so it has to prove the addresses are different. Richard.