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