* [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
@ 2021-04-30 8:30 Alex Coplan
2021-05-17 10:31 ` Alex Coplan
2021-05-17 16:31 ` Richard Earnshaw
0 siblings, 2 replies; 7+ messages in thread
From: Alex Coplan @ 2021-04-30 8:30 UTC (permalink / raw)
To: gcc-patches
Cc: Nick Clifton, Richard Earnshaw, Ramana Radhakrishnan, Kyrylo Tkachov
[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1444 bytes --]
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 45a471a887a..e2ad1a962e3 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"
- "
{
- 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);
+
+ if (tmp)
+ 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();
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
2021-04-30 8:30 [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333] Alex Coplan
@ 2021-05-17 10:31 ` Alex Coplan
2021-05-17 16:31 ` Richard Earnshaw
1 sibling, 0 replies; 7+ messages in thread
From: Alex Coplan @ 2021-05-17 10:31 UTC (permalink / raw)
To: gcc-patches
Cc: Nick Clifton, Richard Earnshaw, Ramana Radhakrishnan, Kyrylo Tkachov
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?
Ping? Bootstrap/regtest looked good, FWIW.
>
> 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.
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 45a471a887a..e2ad1a962e3 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"
> - "
> {
> - 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);
> +
> + if (tmp)
> + 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();
> +}
--
Alex
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
2021-04-30 8:30 [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333] Alex Coplan
2021-05-17 10:31 ` Alex Coplan
@ 2021-05-17 16:31 ` Richard Earnshaw
2021-05-19 14:42 ` Alex Coplan
1 sibling, 1 reply; 7+ messages in thread
From: Richard Earnshaw @ 2021-05-17 16:31 UTC (permalink / raw)
To: Alex Coplan, gcc-patches; +Cc: Richard Earnshaw, Ramana Radhakrishnan
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.
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.
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.
R.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
2021-05-17 16:31 ` Richard Earnshaw
@ 2021-05-19 14:42 ` Alex Coplan
2021-05-19 14:44 ` Alex Coplan
2021-05-19 14:45 ` Richard Earnshaw
0 siblings, 2 replies; 7+ messages in thread
From: Alex Coplan @ 2021-05-19 14:42 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches, Richard Earnshaw, Ramana Radhakrishnan
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
2021-05-19 14:42 ` Alex Coplan
@ 2021-05-19 14:44 ` Alex Coplan
2021-05-19 14:48 ` Richard Earnshaw
2021-05-19 14:45 ` Richard Earnshaw
1 sibling, 1 reply; 7+ messages in thread
From: Alex Coplan @ 2021-05-19 14:44 UTC (permalink / raw)
To: Richard Earnshaw, Richard Earnshaw, gcc-patches, Ramana Radhakrishnan
[-- 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();
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
2021-05-19 14:42 ` Alex Coplan
2021-05-19 14:44 ` Alex Coplan
@ 2021-05-19 14:45 ` Richard Earnshaw
1 sibling, 0 replies; 7+ messages in thread
From: Richard Earnshaw @ 2021-05-19 14:45 UTC (permalink / raw)
To: Alex Coplan; +Cc: Richard Earnshaw, gcc-patches, Ramana Radhakrishnan
ENOATTACHMENT.
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333]
2021-05-19 14:44 ` Alex Coplan
@ 2021-05-19 14:48 ` Richard Earnshaw
0 siblings, 0 replies; 7+ messages in thread
From: Richard Earnshaw @ 2021-05-19 14:48 UTC (permalink / raw)
To: Alex Coplan, Richard Earnshaw, gcc-patches, Ramana Radhakrishnan
On 19/05/2021 15:44, Alex Coplan via Gcc-patches wrote:
> 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
>
OK.
R.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-19 14:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-30 8:30 [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333] Alex Coplan
2021-05-17 10:31 ` Alex Coplan
2021-05-17 16:31 ` Richard Earnshaw
2021-05-19 14:42 ` Alex Coplan
2021-05-19 14:44 ` Alex Coplan
2021-05-19 14:48 ` Richard Earnshaw
2021-05-19 14:45 ` Richard Earnshaw
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).