On 9/11/19 8:30 PM, Richard Biener wrote:
> 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.
>
Hmm, yes, I can just ignore any misaligned MEMs in the equivalence class.
This seems to fix the test case as it seems.
Are we sure that replacing a read from a different alias set cannot cause
problems here? I almost start to think so...
So, how about this?
Thanks
Bernd.
2019-09-11 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/91708
* cse.c (cse_insn): Do not replace anything with a
misaligned MEM.
--- gcc/cse.c.orig 2019-07-24 21:21:53.590065924 +0200
+++ gcc/cse.c 2019-09-11 21:15:03.469388238 +0200
@@ -5385,6 +5385,12 @@ cse_insn (rtx_insn *insn)
/* Do nothing for this case. */
;
+ /* Do not replace anything with a misaligned MEM. */
+ else if (MEM_P (trial)
+ && MEM_ALIGN (trial) < GET_MODE_ALIGNMENT (GET_MODE (trial)))
+ /* Do nothing for this case. */
+ ;
+
/* Look for a substitution that makes a valid insn. */
else if (validate_unshare_change (insn, &SET_SRC (sets[i].rtl),
trial, 0))
