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