This time with attachment.
On 19/05/2021 15:42, Alex Coplan via Gcc-patches wrote:
> 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
--
Alex
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 45a471a887a..064604808cc 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -8580,18 +8580,21 @@ (define_expand "nonsecure_call_internal"
(use (match_operand 2 "" ""))
(clobber (reg:SI LR_REGNUM))])]
"use_cmse"
- "
{
+ rtx addr = XEXP (operands[0], 0);
+ rtx tmp = REG_P (addr) ? addr : force_reg (SImode, addr);
+
if (!TARGET_HAVE_FPCXT_CMSE)
{
- rtx tmp =
- copy_to_suggested_reg (XEXP (operands[0], 0),
- gen_rtx_REG (SImode, R4_REGNUM),
- SImode);
-
- operands[0] = replace_equiv_address (operands[0], tmp);
+ rtx r4 = gen_rtx_REG (SImode, R4_REGNUM);
+ emit_move_insn (r4, tmp);
+ tmp = r4;
}
- }")
+
+ if (tmp != addr)
+ operands[0] = replace_equiv_address (operands[0], tmp);
+ }
+)
(define_insn "*call_reg_armv5"
[(call (mem:SI (match_operand:SI 0 "s_register_operand" "r"))
diff --git a/gcc/testsuite/gcc.target/arm/cmse/pr100333.c
b/gcc/testsuite/gcc.target/arm/cmse/pr100333.c
new file mode 100644
index 00000000000..d8e3d809f73
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/cmse/pr100333.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-mcmse" } */
+typedef void __attribute__((cmse_nonsecure_call)) t(void);
+t g;
+void f() {
+ g();
+}
