On September 11, 2019 4:41:10 PM GMT+02:00, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
>On 9/11/19 3:55 PM, Richard Biener wrote:
>> On Wed, 11 Sep 2019, 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.
>>
>> But why? I see zero benefit doing that. Note the addresses are
>equal,
>> so if it for some reason is beneficial CSE could still simply
>> _preserve_ the original mem_attrs.
>>
>
>Yes, but the addresses are simply *not* equal.
>
>--- cse.c.orig 2019-07-24 21:21:53.590065924 +0200
>+++ cse.c 2019-09-11 16:03:43.429236836 +0200
>@@ -4564,6 +4564,10 @@
> else if (GET_CODE (x) == PARALLEL)
> sets = XALLOCAVEC (struct set, XVECLEN (x, 0));
>
>+ if (INSN_UID(insn) == 234 && GET_CODE(x) == SET
>+ && GET_CODE(SET_DEST(x)) == REG && REGNO(SET_DEST(x)) == 340
>+ && getenv("debug")) asm("int3");
>+
> this_insn = insn;
> /* Records what this insn does to set CC0. */
> this_insn_cc0 = 0;
>
>(gdb) n
>4809 elt = lookup (src, sets[i].src_hash, mode);
>(gdb) call debug(src)
>(mem:SI (reg/v/f:SI 273 [ r ]) [52 MEM <unsigned int> [(struct
>real_value *)r_77(D)]+0 S4 A32])
>(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])
>
>[...]
>5393 else if (validate_unshare_change (insn, &SET_SRC
>(sets[i].rtl),
>(gdb) call debug(insn)
>(insn 234 233 235 11 (set (reg:SI 340 [ MEM <unsigned int> [(struct
>real_value *)r_77(D)] ])
>(mem:SI (reg/v/f:SI 273 [ r ]) [52 MEM <unsigned int> [(struct
>real_value *)r_77(D)]+0 S4 A32])) "../../gcc-trunk-1/gcc/real.c":5185:9
>643 {*thumb2_movsi_vfp}
> (nil))
>(gdb) call debug(trial)
>(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])
>(gdb) n
>5396 rtx new_rtx = canon_reg (SET_SRC (sets[i].rtl), insn);
>(gdb) call debug(insn)
>(insn 234 233 235 11 (set (reg:SI 340 [ MEM <unsigned int> [(struct
>real_value *)r_77(D)] ])
> (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])) "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> (nil))
>
>
>It looks like it is designed to pick a different mem with a different
>address,
>but the same value.
>
>Right?
But the function you originally patched recursed into the MEM operands and thus
the address, comparing them. So I conclude that CSE thinks they have the same
value.
Richard.
>
>Bernd.
