On 9/11/19 7:37 AM, Richard Biener wrote:
> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
>
>> On 9/11/19 12:43 AM, Jeff Law wrote:
>>> On 9/10/19 1:51 PM, 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.
>>>>
>>>> This patch fixes the arm bootstrap for --with-cpu=cortex-a57
>>>> --with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
>>>> which I confirmed using a cross compiler. And it fixes the test case that
>>>> is attached to the PR, but it is way
>>>> too large for the test suite.
>>>>
>>>>
>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>> Is it OK for trunk?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>>
>>>> patch-pr91708.diff
>>>>
>>>> 2019-09-10 Bernd Edlinger <bernd.edlin...@hotmail.de>
>>>>
>>>> PR middle-end/91708
>>>> * cse.c (exp_equiv_p): Consider MEMs with different
>>>> alias set or alignment as different.
>>> Hmm, I would have expected the address space and alignment checks to be
>>> handled by the call to mem_attrs_eq_p.
>>>
>>
>> Yes, but exp_equiv_p is called from here:
>>
>> lookup (rtx x, unsigned int hash, machine_mode mode)
>> {
>> struct table_elt *p;
>>
>> for (p = table[hash]; p; p = p->next_same_hash)
>> if (mode == p->mode && ((x == p->exp && REG_P (x))
>> || exp_equiv_p (x, p->exp, !REG_P (x), false)))
>> return p;
>>
>> with for_gcse = false si mem_attrs_eq_p is not used.
>>
>>> Which aspect of the MEMs is really causing the problem here?
>>>
>>
>> The alignment is (formally) different, but since also the address is
>> different,
>> the alignment could be also be really wrong.
>>
>> Originally it was [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]
>> but it is replaced with [0 MEM <charD.7[1:24]> [(voidD.73 *)r_77(D)]+20 S4
>> A8],
>> that appears to be done, because there is the same value that was in the
>> original
>> location.
>>
>> But also the wrong alias set will cause problems if the instruction
>> is re-materialized in another place (which actually happened in LRA and
>> caused the
>> assertion, BTW).
>
> But !for_gcse shouldn't do this kind of things with MEMs. It should
> only replace a MEM with a REG, not put in some other "equivalent"
> MEMs.
Agreed, generally replacing one MEM with another MEM would be silly.
Though I wouldn't be terribly surprised if CSE did so if the form of one
of the MEMs was cheaper than the other.
It be better to use a REG though.
jeff
