[PATCH] Fix PR 91708

[Richard Biener]

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