Re: [PATCH] Fix PR 91708

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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.


Usually that would be okay, but not when the RTX is extracted from an unspec
unalinged_storedi.


Bernd.


>> 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?
> 
> So no, I don't think this is correct.
> 
> Thanks,
> Richard.
>