Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]

[Alex Coplan <alex.coplan@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]

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

[-- Attachment #2: patch_v2.txt --]
[-- Type: text/plain, Size: 1331 bytes --]

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();
+}