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.
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?
>
> 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?
>
> 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;
