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.
Richard.
