Hi Richard,
On 17/05/2021 17:31, Richard Earnshaw wrote:
>
>
> On 30/04/2021 09:30, Alex Coplan via Gcc-patches wrote:
> > Hi,
> >
> > As the PR shows, we ICE shortly after expanding nonsecure calls for
> > Armv8.1-M. For Armv8.1-M, we have TARGET_HAVE_FPCXT_CMSE. As it stands,
> > the expander (arm.md:nonsecure_call_internal) moves the callee's address
> > to a register (with copy_to_suggested_reg) only if
> > !TARGET_HAVE_FPCXT_CMSE.
> >
> > However, looking at the pattern which the insn appears to be intended to
> > match (thumb2.md:*nonsecure_call_reg_thumb2_fpcxt), it requires the
> > callee's address to be in a register.
> >
> > This patch therefore just forces the callee's address into a register in
> > the expander.
> >
> > Testing:
> > * Regtested an arm-eabi cross configured with
> > --with-arch=armv8.1-m.main+mve.fp+fp.dp --with-float=hard. No regressions.
> > * Bootstrap and regtest on arm-linux-gnueabihf in progress.
> >
> > OK for trunk and backports as appropriate if bootstrap looks good?
> >
> > Thanks,
> > Alex
> >
> > gcc/ChangeLog:
> >
> > PR target/100333
> > * config/arm/arm.md (nonsecure_call_internal): Always ensure
> > callee's address is in a register.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR target/100333
> > * gcc.target/arm/cmse/pr100333.c: New test.
> >
>
>
> - "
> {
> - if (!TARGET_HAVE_FPCXT_CMSE)
> - {
> - rtx tmp =
> - copy_to_suggested_reg (XEXP (operands[0], 0),
> - gen_rtx_REG (SImode, R4_REGNUM),
> - SImode);
> + rtx tmp = NULL_RTX;
> + rtx addr = XEXP (operands[0], 0);
>
> - operands[0] = replace_equiv_address (operands[0], tmp);
> - }
> - }")
> + if (TARGET_HAVE_FPCXT_CMSE && !REG_P (addr))
> + tmp = force_reg (SImode, addr);
> + else if (!TARGET_HAVE_FPCXT_CMSE)
> + tmp = copy_to_suggested_reg (XEXP (operands[0], 0),
> + gen_rtx_REG (SImode, R4_REGNUM),
> + SImode);
>
>
> I think it might be better to handle the !TARGET_HAVE_FPCXT_CMSE case via a
> pseudo as well, then we don't end up generating a potentially non-trivial
> insn that directly writes a fixed hard reg - it's better to let later passes
> clean that up if they can.
Ah, I wasn't aware that was an issue.
>
> Also, you've extracted XEXP (operands[0], 0) into 'addr', but then continue
> to use the XEXP form in the existing path. Please be consistent use XEXP
> directly everywhere, or use 'addr' everywhere.
Fixed, thanks.
>
> So you want something like
>
> addr = XEXP (operands[0], 0);
> if (!REG_P (addr))
> addr = force_reg (SImode, addr);
>
> if (!T_H_F_C)
> addr = copy...(addr, gen(r4), SImode);
>
> operands[0] = replace_equiv_addr (operands[0], addr);
>
> R.
How about the attached? Regtested an armv8.1-m.main cross,
bootstrapped/regtested
on arm-linux-gnueabihf: no issues.
OK for trunk and eventual backports?
Thanks,
Alex
