On September 11, 2019 7:41:22 PM GMT+02:00, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
>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.
But the alias and alignment are properties of the expression representing the
value and have no influence on the value itself.
For expression replacement you usually don't want expression equality but other
checks (even the for_gcse ones show lack of distinction here).
>> 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.
Yes. We can only replace expressions with ones that are obviously safe to
insert here. MEMs are not obviously safe (some might even trap). Regs and
constants are.
Richard.
>
>Bernd.
