On 9/11/19 6:08 PM, Jeff Law wrote:
> 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.
>
The MEMs all have different addresses, but the same value, they are from a
memset or something:
(gdb) call dump_class(elt)
Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM <char[1:24]> [(void
*)r_77(D)]+0 S4 A8]):
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE)
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 20 [0x14])) [0 MEM <char[1:24]> [(void *)r_77(D)]+20 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 16 [0x10])) [0 MEM <char[1:24]> [(void *)r_77(D)]+16 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 12 [0xc])) [0 MEM <char[1:24]> [(void *)r_77(D)]+12 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 8 [0x8])) [0 MEM <char[1:24]> [(void *)r_77(D)]+8 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 4 [0x4])) [0 MEM <char[1:24]> [(void *)r_77(D)]+4 S4 A8])
(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM <char[1:24]> [(void *)r_77(D)]+0 S4 A8])
The insn 234, uses the same address as the last in the list
(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM <char[1:24]> [(void *)r_77(D)]+0 S4 A8])
but incompatible alignment and alias set.
since we only are interested in the value CSE picks the most silly variant,
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 20 [0x14])) [0 MEM <char[1:24]> [(void *)r_77(D)]+20 S4 A8])
now this has a wrong alias set, a wrong alignment and a different address,
and is much more expensive, no wonder that lra needs to rewrite that.
> 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.
>
Yes.
> 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.
>
I think Richi said, replacing MEM with a REG or a CONST would make sense,
We could just try not replace MEM with different MEM.
Bernd.
