This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR 91708
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.edlinger@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.