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 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?
Thanks,
Richard
