On 9/11/19 7:49 AM, Bernd Edlinger wrote:
> On 9/11/19 9:23 AM, Richard Biener wrote:
>> On Tue, 10 Sep 2019, Bernd Edlinger wrote:
>>
>>> Hi!
>>>
>>> This ICE happens when compiling real_nextafter in real.c.
>>> CSE sees this:
>>>
>>> (insn 179 178 180 11 (set (reg:SI 319)
>>> (reg/v/f:SI 273 [ rD.73757 ]))
>>> "../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
>>> (nil))
>>> [...]
>>> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM <charD.7[1:24]>
>>> [(voidD.73 *)r_77(D)]+0 S4 A8])
>>> (unspec:SI [
>>> (reg:SI 320)
>>> ] UNSPEC_UNALIGNED_STORE))
>>> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
>>> (nil))
>>> [...]
>>> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>>> (const_int 20 [0x14])) [0 MEM <charD.7[1:24]> [(voidD.73
>>> *)r_77(D)]+20 S4 A8])
>>> (unspec:SI [
>>> (reg:SI 320)
>>> ] UNSPEC_UNALIGNED_STORE))
>>> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
>>> (expr_list:REG_DEAD (reg:SI 320)
>>> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
>>> (nil))))
>>> [...]
>>> (insn 234 233 235 11 (set (reg:SI 340)
>>> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM <unsigned int>
>>> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>>> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
>>> (nil))
>>>
>>>
>>> ... and transforms insn 234 in an invalid insn:
>>>
>>>
>>> (insn 234 233 235 11 (set (reg:SI 340 [ MEM <unsigned int> [(struct
>>> real_valueD.28367 *)r_77(D)] ])
>>> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>>> (const_int 20 [0x14])) [0 MEM <charD.7[1:24]> [(voidD.73
>>> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
>>> {*thumb2_movsi_vfp}
>>> (nil))
>>>
>>> which triggers the assertion in the arm back-end, because the MEM is not
>>> aligned.
>>>
>>> To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN
>>> or
>>> ALIAS_SET as different.
>>
>> I think the appropriate fix is to retain the mem_attrs of the original
>> MEM since obviously the replacement does not change those? It's a mere
>> change of the MEMs address but implementation-wise CSE replaces the whole
>> MEM?
>>
>> For CSE exp_equiv_p simply compares the addresses, only if for_gcse
>> we do further checks like mem_attrs_eq_p. IMHO that is correct.
>>
>> I'm not sure what CSE does above, that is, what does it think it does?
>> It doesn't replace the loaded value, it seems to do sth else which
>> is odd for CSE (replaces a REG with a PLUS - but why?)
>>
>
> What I think CSE is thinking there is this:
>
> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0 MEM
> <charD.7[1:24]> [(voidD.73 *)r_77(D)]+0 S4 A8]
> that is the same address as where insn 234 reads the value back but there we
> know it is aligned.
>
> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757
> ]) (const_int 20 [0x14])) [0 MEM <charD.7[1:24]> [(voidD.73 *)r_77(D)]+20 S4
> A8]
>
> But since both reg:SI 320 is dead and reg:SI 319 is dead as well, when
> insn 234 processed, it replaces one mem with another mem that has the same
> value.
Just to make sure I understand. Are you saying the addresses for the
MEMs are equal or the contents of the memory location are equal.
For the former the alignment has to be the same, plain and simple, even
if GCC isn't aware the alignments have to be the same.
For the latter we'd be better off loading the data into a REG, then
using the REG for the source of both memory stores.
Jeff
