* [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
@ 2020-06-22 8:51 Andre Vieira (lists)
2020-06-23 12:10 ` Kyrylo Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-06-22 8:51 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]
Hi,
As reported in bugzilla when the -mcmse option is used while compiling
for size (-Os) with a thumb-1 target the generated code will clear the
registers r7-r10. These however are callee saved and should be preserved
accross ABI boundaries. The reason this happens is because these
registers are made "fixed" when optimising for size with Thumb-1 in a
way to make sure they are not used, as pushing and popping hi-registers
requires extra moves to and from LO_REGS.
To fix this, this patch uses 'callee_saved_reg_p', which accounts for
this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
'callee_saved_reg_p''s definition, as it does still take call used
registers into account, which aren't callee_saved in my opinion, so it
is a rather misnoemer, works in our advantage here though as it does
exactly what we need.
Regression tested on arm-none-eabi.
Is this OK for trunk? (Will eventually backport to previous versions if
stable.)
Cheers,
Andre
gcc/ChangeLog:
2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
PR target/95646
* config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
Use 'callee_saved_reg_p' instead of
'calL_used_or_fixed_reg_p'.
gcc/testsuite/ChangeLog:
2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
PR target/95646
* gcc.target/arm/pr95646.c: New test.
[-- Attachment #2: cmse_thumb1_os.patch --]
[-- Type: text/plain, Size: 1342 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 6b7ca829f1c8cbe3d427da474b079882dc522e1a..dac9a6fb5c41ce42cd7a278b417eab25239a043c 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -26960,7 +26960,7 @@ cmse_nonsecure_entry_clear_before_return (void)
continue;
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
- if (call_used_or_fixed_reg_p (regno)
+ if (!callee_saved_reg_p (regno)
&& (!IN_RANGE (regno, FIRST_VFP_REGNUM, LAST_VFP_REGNUM)
|| TARGET_HARD_FLOAT))
bitmap_set_bit (to_clear_bitmap, regno);
diff --git a/gcc/testsuite/gcc.target/arm/pr95646.c b/gcc/testsuite/gcc.target/arm/pr95646.c
new file mode 100644
index 0000000000000000000000000000000000000000..c9fdc37618ccaddcdb597647c7076054af17789a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr95646.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-options "-mcmse -Os -mcpu=cortex-m23" } */
+/* { dg-final { check-function-bodies "**" "" } } */
+
+int __attribute__ ((cmse_nonsecure_entry))
+foo (void)
+{
+ return 1;
+}
+/* { { dg-final { scan-assembler-not "mov\tr9, r0" } } */
+
+/*
+** __acle_se_bar:
+** mov (r[0-3]), r9
+** push {\1}
+** ...
+** pop {(r[0-3])}
+** mov r9, \2
+** ...
+** bxns lr
+*/
+int __attribute__ ((cmse_nonsecure_entry))
+bar (void)
+{
+ asm ("": : : "r9");
+ return 1;
+}
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-22 8:51 [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE Andre Vieira (lists)
@ 2020-06-23 12:10 ` Kyrylo Tkachov
2020-06-23 13:28 ` Andre Vieira (lists)
0 siblings, 1 reply; 14+ messages in thread
From: Kyrylo Tkachov @ 2020-06-23 12:10 UTC (permalink / raw)
To: Andre Simoes Dias Vieira, gcc-patches
> -----Original Message-----
> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> Sent: 22 June 2020 09:52
> To: gcc-patches@gcc.gnu.org
> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
> registers with CMSE
>
> Hi,
>
> As reported in bugzilla when the -mcmse option is used while compiling
> for size (-Os) with a thumb-1 target the generated code will clear the
> registers r7-r10. These however are callee saved and should be preserved
> accross ABI boundaries. The reason this happens is because these
> registers are made "fixed" when optimising for size with Thumb-1 in a
> way to make sure they are not used, as pushing and popping hi-registers
> requires extra moves to and from LO_REGS.
>
> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
> 'callee_saved_reg_p''s definition, as it does still take call used
> registers into account, which aren't callee_saved in my opinion, so it
> is a rather misnoemer, works in our advantage here though as it does
> exactly what we need.
>
> Regression tested on arm-none-eabi.
>
> Is this OK for trunk? (Will eventually backport to previous versions if
> stable.)
Ok.
Thanks,
Kyrill
>
> Cheers,
> Andre
>
> gcc/ChangeLog:
> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> PR target/95646
> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
> Use 'callee_saved_reg_p' instead of
> 'calL_used_or_fixed_reg_p'.
>
> gcc/testsuite/ChangeLog:
> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> PR target/95646
> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-23 12:10 ` Kyrylo Tkachov
@ 2020-06-23 13:28 ` Andre Vieira (lists)
2020-06-23 13:29 ` Kyrylo Tkachov
2020-06-23 20:52 ` Christophe Lyon
0 siblings, 2 replies; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-06-23 13:28 UTC (permalink / raw)
To: Kyrylo Tkachov, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]
On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>
>> -----Original Message-----
>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>> Sent: 22 June 2020 09:52
>> To: gcc-patches@gcc.gnu.org
>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
>> registers with CMSE
>>
>> Hi,
>>
>> As reported in bugzilla when the -mcmse option is used while compiling
>> for size (-Os) with a thumb-1 target the generated code will clear the
>> registers r7-r10. These however are callee saved and should be preserved
>> accross ABI boundaries. The reason this happens is because these
>> registers are made "fixed" when optimising for size with Thumb-1 in a
>> way to make sure they are not used, as pushing and popping hi-registers
>> requires extra moves to and from LO_REGS.
>>
>> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
>> 'callee_saved_reg_p''s definition, as it does still take call used
>> registers into account, which aren't callee_saved in my opinion, so it
>> is a rather misnoemer, works in our advantage here though as it does
>> exactly what we need.
>>
>> Regression tested on arm-none-eabi.
>>
>> Is this OK for trunk? (Will eventually backport to previous versions if
>> stable.)
> Ok.
> Thanks,
> Kyrill
As I was getting ready to push this I noticed I didn't add any skip-ifs
to prevent this failing with specific target options. So here's a new
version with those.
Still OK?
Cheers,
Andre
>
>> Cheers,
>> Andre
>>
>> gcc/ChangeLog:
>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>
>> PR target/95646
>> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
>> Use 'callee_saved_reg_p' instead of
>> 'calL_used_or_fixed_reg_p'.
>>
>> gcc/testsuite/ChangeLog:
>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>
>> PR target/95646
>> * gcc.target/arm/pr95646.c: New test.
[-- Attachment #2: cmse_thumb1_os-2.patch --]
[-- Type: text/plain, Size: 1792 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 6b7ca829f1c8cbe3d427da474b079882dc522e1a..dac9a6fb5c41ce42cd7a278b417eab25239a043c 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -26960,7 +26960,7 @@ cmse_nonsecure_entry_clear_before_return (void)
continue;
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
- if (call_used_or_fixed_reg_p (regno)
+ if (!callee_saved_reg_p (regno)
&& (!IN_RANGE (regno, FIRST_VFP_REGNUM, LAST_VFP_REGNUM)
|| TARGET_HARD_FLOAT))
bitmap_set_bit (to_clear_bitmap, regno);
diff --git a/gcc/testsuite/gcc.target/arm/pr95646.c b/gcc/testsuite/gcc.target/arm/pr95646.c
new file mode 100644
index 0000000000000000000000000000000000000000..12d06a0c8c1ed7de1f8d4d15130432259e613a32
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr95646.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv8-m.base" } } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m23" } } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-mfpu=*" } { } } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=soft" } } */
+/* { dg-options "-mcpu=cortex-m23 -mcmse" } */
+/* { dg-additional-options "-Os" } */
+/* { dg-final { check-function-bodies "**" "" } } */
+
+int __attribute__ ((cmse_nonsecure_entry))
+foo (void)
+{
+ return 1;
+}
+/* { { dg-final { scan-assembler-not "mov\tr9, r0" } } */
+
+/*
+** __acle_se_bar:
+** mov (r[0-3]), r9
+** push {\1}
+** ...
+** pop {(r[0-3])}
+** mov r9, \2
+** ...
+** bxns lr
+*/
+int __attribute__ ((cmse_nonsecure_entry))
+bar (void)
+{
+ asm ("": : : "r9");
+ return 1;
+}
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-23 13:28 ` Andre Vieira (lists)
@ 2020-06-23 13:29 ` Kyrylo Tkachov
2020-06-23 20:52 ` Christophe Lyon
1 sibling, 0 replies; 14+ messages in thread
From: Kyrylo Tkachov @ 2020-06-23 13:29 UTC (permalink / raw)
To: Andre Simoes Dias Vieira, gcc-patches
> -----Original Message-----
> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> Sent: 23 June 2020 14:28
> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
> registers with CMSE
>
> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
> >
> >> -----Original Message-----
> >> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> >> Sent: 22 June 2020 09:52
> >> To: gcc-patches@gcc.gnu.org
> >> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> >> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
> >> registers with CMSE
> >>
> >> Hi,
> >>
> >> As reported in bugzilla when the -mcmse option is used while compiling
> >> for size (-Os) with a thumb-1 target the generated code will clear the
> >> registers r7-r10. These however are callee saved and should be preserved
> >> accross ABI boundaries. The reason this happens is because these
> >> registers are made "fixed" when optimising for size with Thumb-1 in a
> >> way to make sure they are not used, as pushing and popping hi-registers
> >> requires extra moves to and from LO_REGS.
> >>
> >> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
> >> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
> >> 'callee_saved_reg_p''s definition, as it does still take call used
> >> registers into account, which aren't callee_saved in my opinion, so it
> >> is a rather misnoemer, works in our advantage here though as it does
> >> exactly what we need.
> >>
> >> Regression tested on arm-none-eabi.
> >>
> >> Is this OK for trunk? (Will eventually backport to previous versions if
> >> stable.)
> > Ok.
> > Thanks,
> > Kyrill
> As I was getting ready to push this I noticed I didn't add any skip-ifs
> to prevent this failing with specific target options. So here's a new
> version with those.
>
> Still OK?
>
Ok.
Thanks,
Kyrill
> Cheers,
> Andre
> >
> >> Cheers,
> >> Andre
> >>
> >> gcc/ChangeLog:
> >> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>
> >> PR target/95646
> >> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
> >> Use 'callee_saved_reg_p' instead of
> >> 'calL_used_or_fixed_reg_p'.
> >>
> >> gcc/testsuite/ChangeLog:
> >> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>
> >> PR target/95646
> >> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-23 13:28 ` Andre Vieira (lists)
2020-06-23 13:29 ` Kyrylo Tkachov
@ 2020-06-23 20:52 ` Christophe Lyon
2020-06-29 8:45 ` Andre Vieira (lists)
2020-06-29 8:56 ` Andre Vieira (lists)
1 sibling, 2 replies; 14+ messages in thread
From: Christophe Lyon @ 2020-06-23 20:52 UTC (permalink / raw)
To: Andre Vieira (lists); +Cc: Kyrylo Tkachov, gcc-patches
On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
<andre.simoesdiasvieira@arm.com> wrote:
>
> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
> >
> >> -----Original Message-----
> >> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> >> Sent: 22 June 2020 09:52
> >> To: gcc-patches@gcc.gnu.org
> >> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> >> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
> >> registers with CMSE
> >>
> >> Hi,
> >>
> >> As reported in bugzilla when the -mcmse option is used while compiling
> >> for size (-Os) with a thumb-1 target the generated code will clear the
> >> registers r7-r10. These however are callee saved and should be preserved
> >> accross ABI boundaries. The reason this happens is because these
> >> registers are made "fixed" when optimising for size with Thumb-1 in a
> >> way to make sure they are not used, as pushing and popping hi-registers
> >> requires extra moves to and from LO_REGS.
> >>
> >> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
> >> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
> >> 'callee_saved_reg_p''s definition, as it does still take call used
> >> registers into account, which aren't callee_saved in my opinion, so it
> >> is a rather misnoemer, works in our advantage here though as it does
> >> exactly what we need.
> >>
> >> Regression tested on arm-none-eabi.
> >>
> >> Is this OK for trunk? (Will eventually backport to previous versions if
> >> stable.)
> > Ok.
> > Thanks,
> > Kyrill
> As I was getting ready to push this I noticed I didn't add any skip-ifs
> to prevent this failing with specific target options. So here's a new
> version with those.
>
> Still OK?
>
Hi,
This is not sufficient to skip arm-linux-gnueabi* configs built with
non-default cpu/fpu.
For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
--with-fpu=neon-fp16 --with-float=hard
I see:
FAIL: gcc.target/arm/pr95646.c (test for excess errors)
Excess errors:
cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
cc1: error: target CPU does not support ARM mode
and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
Christophe
> Cheers,
> Andre
> >
> >> Cheers,
> >> Andre
> >>
> >> gcc/ChangeLog:
> >> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>
> >> PR target/95646
> >> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
> >> Use 'callee_saved_reg_p' instead of
> >> 'calL_used_or_fixed_reg_p'.
> >>
> >> gcc/testsuite/ChangeLog:
> >> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>
> >> PR target/95646
> >> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-23 20:52 ` Christophe Lyon
@ 2020-06-29 8:45 ` Andre Vieira (lists)
2020-06-29 8:56 ` Andre Vieira (lists)
1 sibling, 0 replies; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-06-29 8:45 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Kyrylo Tkachov, gcc-patches
On 23/06/2020 21:52, Christophe Lyon wrote:
> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
> <andre.simoesdiasvieira@arm.com> wrote:
>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>> -----Original Message-----
>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>> Sent: 22 June 2020 09:52
>>>> To: gcc-patches@gcc.gnu.org
>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
>>>> registers with CMSE
>>>>
>>>> Hi,
>>>>
>>>> As reported in bugzilla when the -mcmse option is used while compiling
>>>> for size (-Os) with a thumb-1 target the generated code will clear the
>>>> registers r7-r10. These however are callee saved and should be preserved
>>>> accross ABI boundaries. The reason this happens is because these
>>>> registers are made "fixed" when optimising for size with Thumb-1 in a
>>>> way to make sure they are not used, as pushing and popping hi-registers
>>>> requires extra moves to and from LO_REGS.
>>>>
>>>> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
>>>> 'callee_saved_reg_p''s definition, as it does still take call used
>>>> registers into account, which aren't callee_saved in my opinion, so it
>>>> is a rather misnoemer, works in our advantage here though as it does
>>>> exactly what we need.
>>>>
>>>> Regression tested on arm-none-eabi.
>>>>
>>>> Is this OK for trunk? (Will eventually backport to previous versions if
>>>> stable.)
>>> Ok.
>>> Thanks,
>>> Kyrill
>> As I was getting ready to push this I noticed I didn't add any skip-ifs
>> to prevent this failing with specific target options. So here's a new
>> version with those.
>>
>> Still OK?
>>
> Hi,
>
> This is not sufficient to skip arm-linux-gnueabi* configs built with
> non-default cpu/fpu.
>
> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
> --with-fpu=neon-fp16 --with-float=hard
> I see:
> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
> Excess errors:
> cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
> cc1: error: target CPU does not support ARM mode
>
> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
I'm not following this, before I go off and try to reproduce it, what do
you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os'?
These are the options you are seeing in the log file? Surely they should
override the default options? Only thing I can think of is this might
need an extra -mfloat-abi=soft to make sure it overrides the default
float-abi. Could you give that a try?
Cheers,
Andre
>
> Christophe
>
>> Cheers,
>> Andre
>>>> Cheers,
>>>> Andre
>>>>
>>>> gcc/ChangeLog:
>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>
>>>> PR target/95646
>>>> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
>>>> Use 'callee_saved_reg_p' instead of
>>>> 'calL_used_or_fixed_reg_p'.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>
>>>> PR target/95646
>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-23 20:52 ` Christophe Lyon
2020-06-29 8:45 ` Andre Vieira (lists)
@ 2020-06-29 8:56 ` Andre Vieira (lists)
2020-06-29 10:15 ` Christophe Lyon
1 sibling, 1 reply; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-06-29 8:56 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Kyrylo Tkachov, gcc-patches
On 23/06/2020 21:52, Christophe Lyon wrote:
> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
> <andre.simoesdiasvieira@arm.com> wrote:
>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>> -----Original Message-----
>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>> Sent: 22 June 2020 09:52
>>>> To: gcc-patches@gcc.gnu.org
>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
>>>> registers with CMSE
>>>>
>>>> Hi,
>>>>
>>>> As reported in bugzilla when the -mcmse option is used while compiling
>>>> for size (-Os) with a thumb-1 target the generated code will clear the
>>>> registers r7-r10. These however are callee saved and should be preserved
>>>> accross ABI boundaries. The reason this happens is because these
>>>> registers are made "fixed" when optimising for size with Thumb-1 in a
>>>> way to make sure they are not used, as pushing and popping hi-registers
>>>> requires extra moves to and from LO_REGS.
>>>>
>>>> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
>>>> 'callee_saved_reg_p''s definition, as it does still take call used
>>>> registers into account, which aren't callee_saved in my opinion, so it
>>>> is a rather misnoemer, works in our advantage here though as it does
>>>> exactly what we need.
>>>>
>>>> Regression tested on arm-none-eabi.
>>>>
>>>> Is this OK for trunk? (Will eventually backport to previous versions if
>>>> stable.)
>>> Ok.
>>> Thanks,
>>> Kyrill
>> As I was getting ready to push this I noticed I didn't add any skip-ifs
>> to prevent this failing with specific target options. So here's a new
>> version with those.
>>
>> Still OK?
>>
> Hi,
>
> This is not sufficient to skip arm-linux-gnueabi* configs built with
> non-default cpu/fpu.
>
> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
> --with-fpu=neon-fp16 --with-float=hard
> I see:
> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
> Excess errors:
> cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
> cc1: error: target CPU does not support ARM mode
>
> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
Resending as I don't think my earlier one made it to the lists (sorry if
you are receiving this double!)
I'm not following this, before I go off and try to reproduce it, what do
you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os'?
These are the options you are seeing in the log file? Surely they should
override the default options? Only thing I can think of is this might
need an extra -mfloat-abi=soft to make sure it overrides the default
float-abi. Could you give that a try?
Cheers,
Andre
>
> Christophe
>
>> Cheers,
>> Andre
>>>> Cheers,
>>>> Andre
>>>>
>>>> gcc/ChangeLog:
>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>
>>>> PR target/95646
>>>> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
>>>> Use 'callee_saved_reg_p' instead of
>>>> 'calL_used_or_fixed_reg_p'.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>
>>>> PR target/95646
>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-29 8:56 ` Andre Vieira (lists)
@ 2020-06-29 10:15 ` Christophe Lyon
2020-06-30 13:50 ` Andre Vieira (lists)
0 siblings, 1 reply; 14+ messages in thread
From: Christophe Lyon @ 2020-06-29 10:15 UTC (permalink / raw)
To: Andre Vieira (lists); +Cc: Kyrylo Tkachov, gcc-patches
On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
<andre.simoesdiasvieira@arm.com> wrote:
>
>
> On 23/06/2020 21:52, Christophe Lyon wrote:
> > On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
> > <andre.simoesdiasvieira@arm.com> wrote:
> >> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
> >>>> -----Original Message-----
> >>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> >>>> Sent: 22 June 2020 09:52
> >>>> To: gcc-patches@gcc.gnu.org
> >>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> >>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
> >>>> registers with CMSE
> >>>>
> >>>> Hi,
> >>>>
> >>>> As reported in bugzilla when the -mcmse option is used while compiling
> >>>> for size (-Os) with a thumb-1 target the generated code will clear the
> >>>> registers r7-r10. These however are callee saved and should be preserved
> >>>> accross ABI boundaries. The reason this happens is because these
> >>>> registers are made "fixed" when optimising for size with Thumb-1 in a
> >>>> way to make sure they are not used, as pushing and popping hi-registers
> >>>> requires extra moves to and from LO_REGS.
> >>>>
> >>>> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
> >>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
> >>>> 'callee_saved_reg_p''s definition, as it does still take call used
> >>>> registers into account, which aren't callee_saved in my opinion, so it
> >>>> is a rather misnoemer, works in our advantage here though as it does
> >>>> exactly what we need.
> >>>>
> >>>> Regression tested on arm-none-eabi.
> >>>>
> >>>> Is this OK for trunk? (Will eventually backport to previous versions if
> >>>> stable.)
> >>> Ok.
> >>> Thanks,
> >>> Kyrill
> >> As I was getting ready to push this I noticed I didn't add any skip-ifs
> >> to prevent this failing with specific target options. So here's a new
> >> version with those.
> >>
> >> Still OK?
> >>
> > Hi,
> >
> > This is not sufficient to skip arm-linux-gnueabi* configs built with
> > non-default cpu/fpu.
> >
> > For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
> > --with-fpu=neon-fp16 --with-float=hard
> > I see:
> > FAIL: gcc.target/arm/pr95646.c (test for excess errors)
> > Excess errors:
> > cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
> > cc1: error: target CPU does not support ARM mode
> >
> > and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
> Resending as I don't think my earlier one made it to the lists (sorry if
> you are receiving this double!)
>
> I'm not following this, before I go off and try to reproduce it, what do
> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os'?
> These are the options you are seeing in the log file? Surely they should
> override the default options? Only thing I can think of is this might
> need an extra -mfloat-abi=soft to make sure it overrides the default
> float-abi. Could you give that a try?
No it doesn't make a difference alone.
I also had to add:
-mfpu=auto (that clears the above warning)
-mthumb otherwise we now get cc1: error: target CPU does not support ARM mode
Looks like some effective-target machinery is needed
Christophe
>
> Cheers,
> Andre
> >
> > Christophe
> >
> >> Cheers,
> >> Andre
> >>>> Cheers,
> >>>> Andre
> >>>>
> >>>> gcc/ChangeLog:
> >>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>>>
> >>>> PR target/95646
> >>>> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
> >>>> Use 'callee_saved_reg_p' instead of
> >>>> 'calL_used_or_fixed_reg_p'.
> >>>>
> >>>> gcc/testsuite/ChangeLog:
> >>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>>>
> >>>> PR target/95646
> >>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-29 10:15 ` Christophe Lyon
@ 2020-06-30 13:50 ` Andre Vieira (lists)
2020-07-06 14:30 ` Andre Vieira (lists)
0 siblings, 1 reply; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-06-30 13:50 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Kyrylo Tkachov, gcc-patches
On 29/06/2020 11:15, Christophe Lyon wrote:
> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
> <andre.simoesdiasvieira@arm.com> wrote:
>>
>> On 23/06/2020 21:52, Christophe Lyon wrote:
>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>>>> -----Original Message-----
>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>>>> Sent: 22 June 2020 09:52
>>>>>> To: gcc-patches@gcc.gnu.org
>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved
>>>>>> registers with CMSE
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> As reported in bugzilla when the -mcmse option is used while compiling
>>>>>> for size (-Os) with a thumb-1 target the generated code will clear the
>>>>>> registers r7-r10. These however are callee saved and should be preserved
>>>>>> accross ABI boundaries. The reason this happens is because these
>>>>>> registers are made "fixed" when optimising for size with Thumb-1 in a
>>>>>> way to make sure they are not used, as pushing and popping hi-registers
>>>>>> requires extra moves to and from LO_REGS.
>>>>>>
>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which accounts for
>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be aware of
>>>>>> 'callee_saved_reg_p''s definition, as it does still take call used
>>>>>> registers into account, which aren't callee_saved in my opinion, so it
>>>>>> is a rather misnoemer, works in our advantage here though as it does
>>>>>> exactly what we need.
>>>>>>
>>>>>> Regression tested on arm-none-eabi.
>>>>>>
>>>>>> Is this OK for trunk? (Will eventually backport to previous versions if
>>>>>> stable.)
>>>>> Ok.
>>>>> Thanks,
>>>>> Kyrill
>>>> As I was getting ready to push this I noticed I didn't add any skip-ifs
>>>> to prevent this failing with specific target options. So here's a new
>>>> version with those.
>>>>
>>>> Still OK?
>>>>
>>> Hi,
>>>
>>> This is not sufficient to skip arm-linux-gnueabi* configs built with
>>> non-default cpu/fpu.
>>>
>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
>>> --with-fpu=neon-fp16 --with-float=hard
>>> I see:
>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
>>> Excess errors:
>>> cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
>>> cc1: error: target CPU does not support ARM mode
>>>
>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
>> Resending as I don't think my earlier one made it to the lists (sorry if
>> you are receiving this double!)
>>
>> I'm not following this, before I go off and try to reproduce it, what do
>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os'?
>> These are the options you are seeing in the log file? Surely they should
>> override the default options? Only thing I can think of is this might
>> need an extra -mfloat-abi=soft to make sure it overrides the default
>> float-abi. Could you give that a try?
> No it doesn't make a difference alone.
>
> I also had to add:
> -mfpu=auto (that clears the above warning)
> -mthumb otherwise we now get cc1: error: target CPU does not support ARM mode
>
> Looks like some effective-target machinery is needed
So I had a look at this, I was pretty sure that -mfloat-abi=soft
overwrote -mfpu=<>, which in large it does, as in no FP instructions
will be generated but the error you see only checks for the right number
of FP registers. Which doesn't check whether 'TARGET_HARD_FLOAT' is set
or not. I'll fix this too and use the check-effective-target for
armv8-m.base for this test as it is indeed a better approach than my bag
of skip-ifs. I'm testing it locally to make sure my changes don't break
anything.
Cheers,
Andre
>
> Christophe
>
>
>> Cheers,
>> Andre
>>> Christophe
>>>
>>>> Cheers,
>>>> Andre
>>>>>> Cheers,
>>>>>> Andre
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>
>>>>>> PR target/95646
>>>>>> * config/arm/arm.c: (cmse_nonsecure_entry_clear_before_return):
>>>>>> Use 'callee_saved_reg_p' instead of
>>>>>> 'calL_used_or_fixed_reg_p'.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>
>>>>>> PR target/95646
>>>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-06-30 13:50 ` Andre Vieira (lists)
@ 2020-07-06 14:30 ` Andre Vieira (lists)
2020-07-07 12:43 ` Christophe Lyon
2020-08-06 9:56 ` Kyrylo Tkachov
0 siblings, 2 replies; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-07-06 14:30 UTC (permalink / raw)
To: gcc-patches, Christophe Lyon
[-- Attachment #1: Type: text/plain, Size: 5639 bytes --]
On 30/06/2020 14:50, Andre Vieira (lists) wrote:
>
> On 29/06/2020 11:15, Christophe Lyon wrote:
>> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
>> <andre.simoesdiasvieira@arm.com> wrote:
>>>
>>> On 23/06/2020 21:52, Christophe Lyon wrote:
>>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>>>>> Sent: 22 June 2020 09:52
>>>>>>> To: gcc-patches@gcc.gnu.org
>>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber
>>>>>>> callee saved
>>>>>>> registers with CMSE
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> As reported in bugzilla when the -mcmse option is used while
>>>>>>> compiling
>>>>>>> for size (-Os) with a thumb-1 target the generated code will
>>>>>>> clear the
>>>>>>> registers r7-r10. These however are callee saved and should be
>>>>>>> preserved
>>>>>>> accross ABI boundaries. The reason this happens is because these
>>>>>>> registers are made "fixed" when optimising for size with Thumb-1
>>>>>>> in a
>>>>>>> way to make sure they are not used, as pushing and popping
>>>>>>> hi-registers
>>>>>>> requires extra moves to and from LO_REGS.
>>>>>>>
>>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which
>>>>>>> accounts for
>>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be
>>>>>>> aware of
>>>>>>> 'callee_saved_reg_p''s definition, as it does still take call used
>>>>>>> registers into account, which aren't callee_saved in my opinion,
>>>>>>> so it
>>>>>>> is a rather misnoemer, works in our advantage here though as it
>>>>>>> does
>>>>>>> exactly what we need.
>>>>>>>
>>>>>>> Regression tested on arm-none-eabi.
>>>>>>>
>>>>>>> Is this OK for trunk? (Will eventually backport to previous
>>>>>>> versions if
>>>>>>> stable.)
>>>>>> Ok.
>>>>>> Thanks,
>>>>>> Kyrill
>>>>> As I was getting ready to push this I noticed I didn't add any
>>>>> skip-ifs
>>>>> to prevent this failing with specific target options. So here's a new
>>>>> version with those.
>>>>>
>>>>> Still OK?
>>>>>
>>>> Hi,
>>>>
>>>> This is not sufficient to skip arm-linux-gnueabi* configs built with
>>>> non-default cpu/fpu.
>>>>
>>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
>>>> --with-fpu=neon-fp16 --with-float=hard
>>>> I see:
>>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
>>>> Excess errors:
>>>> cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
>>>> cc1: error: target CPU does not support ARM mode
>>>>
>>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
>>> Resending as I don't think my earlier one made it to the lists
>>> (sorry if
>>> you are receiving this double!)
>>>
>>> I'm not following this, before I go off and try to reproduce it,
>>> what do
>>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse
>>> -Os'?
>>> These are the options you are seeing in the log file? Surely they
>>> should
>>> override the default options? Only thing I can think of is this might
>>> need an extra -mfloat-abi=soft to make sure it overrides the default
>>> float-abi. Could you give that a try?
>> No it doesn't make a difference alone.
>>
>> I also had to add:
>> -mfpu=auto (that clears the above warning)
>> -mthumb otherwise we now get cc1: error: target CPU does not support
>> ARM mode
>>
>> Looks like some effective-target machinery is needed
> So I had a look at this, I was pretty sure that -mfloat-abi=soft
> overwrote -mfpu=<>, which in large it does, as in no FP instructions
> will be generated but the error you see only checks for the right
> number of FP registers. Which doesn't check whether
> 'TARGET_HARD_FLOAT' is set or not. I'll fix this too and use the
> check-effective-target for armv8-m.base for this test as it is indeed
> a better approach than my bag of skip-ifs. I'm testing it locally to
> make sure my changes don't break anything.
>
> Cheers,
> Andre
Hi,
Sorry for the delay. So I changed the test to use the effective-target
machinery as you suggested and I also made sure that you don't get the
"ARMv8-M Security Extensions incompatible with selected FPU" when
-mfloat-abi=soft.
Further changed 'asm' to '__asm__' to avoid failures with '-std=' options.
Regression tested on arm-none-eabi.
@Christophe: could you test this for your configuration, shouldn't fail
anymore!
Is this OK for trunk?
Cheers,
Andre
gcc/ChangeLog:
2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
* config/arm/arm.c (arm_options_perform_arch_sanity_checks): Do not
check +D32 for CMSE if -mfloat-abi=soft
gcc/testsuite/ChangeLog:
2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
* gcc.target/arm/pr95646.c: Fix testism.
>>
>> Christophe
>>
>>
>>> Cheers,
>>> Andre
>>>> Christophe
>>>>
>>>>> Cheers,
>>>>> Andre
>>>>>>> Cheers,
>>>>>>> Andre
>>>>>>>
>>>>>>> gcc/ChangeLog:
>>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>
>>>>>>> PR target/95646
>>>>>>> * config/arm/arm.c:
>>>>>>> (cmse_nonsecure_entry_clear_before_return):
>>>>>>> Use 'callee_saved_reg_p' instead of
>>>>>>> 'calL_used_or_fixed_reg_p'.
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>
>>>>>>> PR target/95646
>>>>>>> * gcc.target/arm/pr95646.c: New test.
[-- Attachment #2: pr95646_testism_fix.patch --]
[-- Type: text/plain, Size: 1873 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index dac9a6fb5c41ce42cd7a278b417eab25239a043c..38500220bfb2a1ddbbff15eb552451701f7256d5 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3834,7 +3834,7 @@ arm_options_perform_arch_sanity_checks (void)
/* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
and ARMv8-M Baseline and Mainline do not allow such configuration. */
- if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ if (use_cmse && TARGET_HARD_FLOAT && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
error ("ARMv8-M Security Extensions incompatible with selected FPU");
diff --git a/gcc/testsuite/gcc.target/arm/pr95646.c b/gcc/testsuite/gcc.target/arm/pr95646.c
index 12d06a0c8c1ed7de1f8d4d15130432259e613a32..cde1b2d9d36a4e39cd916fdcc9eef424a22bd589 100644
--- a/gcc/testsuite/gcc.target/arm/pr95646.c
+++ b/gcc/testsuite/gcc.target/arm/pr95646.c
@@ -1,10 +1,7 @@
/* { dg-do compile } */
-/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv8-m.base" } } */
-/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m23" } } */
-/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-mfpu=*" } { } } */
-/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=soft" } } */
-/* { dg-options "-mcpu=cortex-m23 -mcmse" } */
-/* { dg-additional-options "-Os" } */
+/* { dg-require-effective-target arm_arch_v8m_base_ok } */
+/* { dg-add-options arm_arch_v8m_base } */
+/* { dg-additional-options "-mcmse -Os" } */
/* { dg-final { check-function-bodies "**" "" } } */
int __attribute__ ((cmse_nonsecure_entry))
@@ -27,6 +24,6 @@ foo (void)
int __attribute__ ((cmse_nonsecure_entry))
bar (void)
{
- asm ("": : : "r9");
+ __asm__ ("" : : : "r9");
return 1;
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-07-06 14:30 ` Andre Vieira (lists)
@ 2020-07-07 12:43 ` Christophe Lyon
2020-07-08 8:04 ` Andre Simoes Dias Vieira
2020-08-06 9:56 ` Kyrylo Tkachov
1 sibling, 1 reply; 14+ messages in thread
From: Christophe Lyon @ 2020-07-07 12:43 UTC (permalink / raw)
To: Andre Vieira (lists); +Cc: gcc Patches, Kyrylo Tkachov
Hi,
On Mon, 6 Jul 2020 at 16:31, Andre Vieira (lists)
<andre.simoesdiasvieira@arm.com> wrote:
>
>
> On 30/06/2020 14:50, Andre Vieira (lists) wrote:
> >
> > On 29/06/2020 11:15, Christophe Lyon wrote:
> >> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
> >> <andre.simoesdiasvieira@arm.com> wrote:
> >>>
> >>> On 23/06/2020 21:52, Christophe Lyon wrote:
> >>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
> >>>> <andre.simoesdiasvieira@arm.com> wrote:
> >>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> >>>>>>> Sent: 22 June 2020 09:52
> >>>>>>> To: gcc-patches@gcc.gnu.org
> >>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> >>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber
> >>>>>>> callee saved
> >>>>>>> registers with CMSE
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> As reported in bugzilla when the -mcmse option is used while
> >>>>>>> compiling
> >>>>>>> for size (-Os) with a thumb-1 target the generated code will
> >>>>>>> clear the
> >>>>>>> registers r7-r10. These however are callee saved and should be
> >>>>>>> preserved
> >>>>>>> accross ABI boundaries. The reason this happens is because these
> >>>>>>> registers are made "fixed" when optimising for size with Thumb-1
> >>>>>>> in a
> >>>>>>> way to make sure they are not used, as pushing and popping
> >>>>>>> hi-registers
> >>>>>>> requires extra moves to and from LO_REGS.
> >>>>>>>
> >>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which
> >>>>>>> accounts for
> >>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be
> >>>>>>> aware of
> >>>>>>> 'callee_saved_reg_p''s definition, as it does still take call used
> >>>>>>> registers into account, which aren't callee_saved in my opinion,
> >>>>>>> so it
> >>>>>>> is a rather misnoemer, works in our advantage here though as it
> >>>>>>> does
> >>>>>>> exactly what we need.
> >>>>>>>
> >>>>>>> Regression tested on arm-none-eabi.
> >>>>>>>
> >>>>>>> Is this OK for trunk? (Will eventually backport to previous
> >>>>>>> versions if
> >>>>>>> stable.)
> >>>>>> Ok.
> >>>>>> Thanks,
> >>>>>> Kyrill
> >>>>> As I was getting ready to push this I noticed I didn't add any
> >>>>> skip-ifs
> >>>>> to prevent this failing with specific target options. So here's a new
> >>>>> version with those.
> >>>>>
> >>>>> Still OK?
> >>>>>
> >>>> Hi,
> >>>>
> >>>> This is not sufficient to skip arm-linux-gnueabi* configs built with
> >>>> non-default cpu/fpu.
> >>>>
> >>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
> >>>> --with-fpu=neon-fp16 --with-float=hard
> >>>> I see:
> >>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
> >>>> Excess errors:
> >>>> cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
> >>>> cc1: error: target CPU does not support ARM mode
> >>>>
> >>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
> >>> Resending as I don't think my earlier one made it to the lists
> >>> (sorry if
> >>> you are receiving this double!)
> >>>
> >>> I'm not following this, before I go off and try to reproduce it,
> >>> what do
> >>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse
> >>> -Os'?
> >>> These are the options you are seeing in the log file? Surely they
> >>> should
> >>> override the default options? Only thing I can think of is this might
> >>> need an extra -mfloat-abi=soft to make sure it overrides the default
> >>> float-abi. Could you give that a try?
> >> No it doesn't make a difference alone.
> >>
> >> I also had to add:
> >> -mfpu=auto (that clears the above warning)
> >> -mthumb otherwise we now get cc1: error: target CPU does not support
> >> ARM mode
> >>
> >> Looks like some effective-target machinery is needed
> > So I had a look at this, I was pretty sure that -mfloat-abi=soft
> > overwrote -mfpu=<>, which in large it does, as in no FP instructions
> > will be generated but the error you see only checks for the right
> > number of FP registers. Which doesn't check whether
> > 'TARGET_HARD_FLOAT' is set or not. I'll fix this too and use the
> > check-effective-target for armv8-m.base for this test as it is indeed
> > a better approach than my bag of skip-ifs. I'm testing it locally to
> > make sure my changes don't break anything.
> >
> > Cheers,
> > Andre
> Hi,
>
> Sorry for the delay. So I changed the test to use the effective-target
> machinery as you suggested and I also made sure that you don't get the
> "ARMv8-M Security Extensions incompatible with selected FPU" when
> -mfloat-abi=soft.
> Further changed 'asm' to '__asm__' to avoid failures with '-std=' options.
>
> Regression tested on arm-none-eabi.
> @Christophe: could you test this for your configuration, shouldn't fail
> anymore!
>
Indeed with your patch I don't see any failure with pr95646.c
Note that it is still unsupported with arm-eabi when running the tests
with -mcpu=cortex-mXX
because the compiler complains that -mcpu=cortex-mXX conflicts with
-march=armv8-m.base,
thus the effective-target test fails.
BTW, is that warning useful/practical? Wouldn't it be more convenient
if the last -mcpu/-march
on the command line was the only one taken into account? (I had a
similar issue when
running tests (libstdc++) getting -march=armv8-m.main+fp from their
multilib environment
and forcing -mcpu=cortex-m33 because it also means '+dsp' and produces
a warning;
I had to use -mcpu=cortex-m33 -march=armv8-m.main+fp+dsp to workaround this)
It passes with the config I test without overriding runtestflags flags
though, so it's covered.
Thanks
Christophe
> Is this OK for trunk?
>
> Cheers,
> Andre
>
> gcc/ChangeLog:
> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> * config/arm/arm.c (arm_options_perform_arch_sanity_checks): Do not
> check +D32 for CMSE if -mfloat-abi=soft
>
> gcc/testsuite/ChangeLog:
> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> * gcc.target/arm/pr95646.c: Fix testism.
> >>
> >> Christophe
> >>
> >>
> >>> Cheers,
> >>> Andre
> >>>> Christophe
> >>>>
> >>>>> Cheers,
> >>>>> Andre
> >>>>>>> Cheers,
> >>>>>>> Andre
> >>>>>>>
> >>>>>>> gcc/ChangeLog:
> >>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>>>>>>
> >>>>>>> PR target/95646
> >>>>>>> * config/arm/arm.c:
> >>>>>>> (cmse_nonsecure_entry_clear_before_return):
> >>>>>>> Use 'callee_saved_reg_p' instead of
> >>>>>>> 'calL_used_or_fixed_reg_p'.
> >>>>>>>
> >>>>>>> gcc/testsuite/ChangeLog:
> >>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>>>>>>
> >>>>>>> PR target/95646
> >>>>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-07-07 12:43 ` Christophe Lyon
@ 2020-07-08 8:04 ` Andre Simoes Dias Vieira
2020-07-20 13:33 ` Andre Vieira (lists)
0 siblings, 1 reply; 14+ messages in thread
From: Andre Simoes Dias Vieira @ 2020-07-08 8:04 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc Patches, Kyrylo Tkachov
On 07/07/2020 13:43, Christophe Lyon wrote:
> Hi,
>
>
> On Mon, 6 Jul 2020 at 16:31, Andre Vieira (lists)
> <andre.simoesdiasvieira@arm.com> wrote:
>>
>> On 30/06/2020 14:50, Andre Vieira (lists) wrote:
>>> On 29/06/2020 11:15, Christophe Lyon wrote:
>>>> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>> On 23/06/2020 21:52, Christophe Lyon wrote:
>>>>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
>>>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>>>>>>> Sent: 22 June 2020 09:52
>>>>>>>>> To: gcc-patches@gcc.gnu.org
>>>>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber
>>>>>>>>> callee saved
>>>>>>>>> registers with CMSE
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> As reported in bugzilla when the -mcmse option is used while
>>>>>>>>> compiling
>>>>>>>>> for size (-Os) with a thumb-1 target the generated code will
>>>>>>>>> clear the
>>>>>>>>> registers r7-r10. These however are callee saved and should be
>>>>>>>>> preserved
>>>>>>>>> accross ABI boundaries. The reason this happens is because these
>>>>>>>>> registers are made "fixed" when optimising for size with Thumb-1
>>>>>>>>> in a
>>>>>>>>> way to make sure they are not used, as pushing and popping
>>>>>>>>> hi-registers
>>>>>>>>> requires extra moves to and from LO_REGS.
>>>>>>>>>
>>>>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which
>>>>>>>>> accounts for
>>>>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be
>>>>>>>>> aware of
>>>>>>>>> 'callee_saved_reg_p''s definition, as it does still take call used
>>>>>>>>> registers into account, which aren't callee_saved in my opinion,
>>>>>>>>> so it
>>>>>>>>> is a rather misnoemer, works in our advantage here though as it
>>>>>>>>> does
>>>>>>>>> exactly what we need.
>>>>>>>>>
>>>>>>>>> Regression tested on arm-none-eabi.
>>>>>>>>>
>>>>>>>>> Is this OK for trunk? (Will eventually backport to previous
>>>>>>>>> versions if
>>>>>>>>> stable.)
>>>>>>>> Ok.
>>>>>>>> Thanks,
>>>>>>>> Kyrill
>>>>>>> As I was getting ready to push this I noticed I didn't add any
>>>>>>> skip-ifs
>>>>>>> to prevent this failing with specific target options. So here's a new
>>>>>>> version with those.
>>>>>>>
>>>>>>> Still OK?
>>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> This is not sufficient to skip arm-linux-gnueabi* configs built with
>>>>>> non-default cpu/fpu.
>>>>>>
>>>>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
>>>>>> --with-fpu=neon-fp16 --with-float=hard
>>>>>> I see:
>>>>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
>>>>>> Excess errors:
>>>>>> cc1: error: ARMv8-M Security Extensions incompatible with selected FPU
>>>>>> cc1: error: target CPU does not support ARM mode
>>>>>>
>>>>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
>>>>> Resending as I don't think my earlier one made it to the lists
>>>>> (sorry if
>>>>> you are receiving this double!)
>>>>>
>>>>> I'm not following this, before I go off and try to reproduce it,
>>>>> what do
>>>>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse
>>>>> -Os'?
>>>>> These are the options you are seeing in the log file? Surely they
>>>>> should
>>>>> override the default options? Only thing I can think of is this might
>>>>> need an extra -mfloat-abi=soft to make sure it overrides the default
>>>>> float-abi. Could you give that a try?
>>>> No it doesn't make a difference alone.
>>>>
>>>> I also had to add:
>>>> -mfpu=auto (that clears the above warning)
>>>> -mthumb otherwise we now get cc1: error: target CPU does not support
>>>> ARM mode
>>>>
>>>> Looks like some effective-target machinery is needed
>>> So I had a look at this, I was pretty sure that -mfloat-abi=soft
>>> overwrote -mfpu=<>, which in large it does, as in no FP instructions
>>> will be generated but the error you see only checks for the right
>>> number of FP registers. Which doesn't check whether
>>> 'TARGET_HARD_FLOAT' is set or not. I'll fix this too and use the
>>> check-effective-target for armv8-m.base for this test as it is indeed
>>> a better approach than my bag of skip-ifs. I'm testing it locally to
>>> make sure my changes don't break anything.
>>>
>>> Cheers,
>>> Andre
>> Hi,
>>
>> Sorry for the delay. So I changed the test to use the effective-target
>> machinery as you suggested and I also made sure that you don't get the
>> "ARMv8-M Security Extensions incompatible with selected FPU" when
>> -mfloat-abi=soft.
>> Further changed 'asm' to '__asm__' to avoid failures with '-std=' options.
>>
>> Regression tested on arm-none-eabi.
>> @Christophe: could you test this for your configuration, shouldn't fail
>> anymore!
>>
> Indeed with your patch I don't see any failure with pr95646.c
>
> Note that it is still unsupported with arm-eabi when running the tests
> with -mcpu=cortex-mXX
> because the compiler complains that -mcpu=cortex-mXX conflicts with
> -march=armv8-m.base,
> thus the effective-target test fails.
>
> BTW, is that warning useful/practical? Wouldn't it be more convenient
> if the last -mcpu/-march
> on the command line was the only one taken into account? (I had a
> similar issue when
> running tests (libstdc++) getting -march=armv8-m.main+fp from their
> multilib environment
> and forcing -mcpu=cortex-m33 because it also means '+dsp' and produces
> a warning;
> I had to use -mcpu=cortex-m33 -march=armv8-m.main+fp+dsp to workaround this)
Yeah I've been annoyed by that before, also in the context of testing
multilibs.
Even though I can see how it can be a useful warning though, if you are
using these in build-systems and you accidentally introduce a new
(incompatible) -mcpu/-march alongside the old one. Though it seems
arbitrary, as we do not warn against multiple -mcpu's or -march's...
I would be in favor of introducing a flag to turn off the warning, such
that we don't have to worry about it in the testsuite. Unfortunately, it
seems -march always wins, regardless of order .... Meaning that if we do
add and use such a warning-off option, we would also have to always use
'-march' as additional-options when creating testcases, to ensure it
overrides any RUNTESTFLAG incompatible -mcpu or different -march options.
Either way, its a bit of a mess that could probably do with some
cleaning up. One thing at a time though.
>
> It passes with the config I test without overriding runtestflags flags
> though, so it's covered.
>
> Thanks
>
> Christophe
>
>> Is this OK for trunk?
>>
>> Cheers,
>> Andre
>>
>> gcc/ChangeLog:
>> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>
>> * config/arm/arm.c (arm_options_perform_arch_sanity_checks): Do not
>> check +D32 for CMSE if -mfloat-abi=soft
>>
>> gcc/testsuite/ChangeLog:
>> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>
>> * gcc.target/arm/pr95646.c: Fix testism.
>>>> Christophe
>>>>
>>>>
>>>>> Cheers,
>>>>> Andre
>>>>>> Christophe
>>>>>>
>>>>>>> Cheers,
>>>>>>> Andre
>>>>>>>>> Cheers,
>>>>>>>>> Andre
>>>>>>>>>
>>>>>>>>> gcc/ChangeLog:
>>>>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>>>
>>>>>>>>> PR target/95646
>>>>>>>>> * config/arm/arm.c:
>>>>>>>>> (cmse_nonsecure_entry_clear_before_return):
>>>>>>>>> Use 'callee_saved_reg_p' instead of
>>>>>>>>> 'calL_used_or_fixed_reg_p'.
>>>>>>>>>
>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>>>
>>>>>>>>> PR target/95646
>>>>>>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-07-08 8:04 ` Andre Simoes Dias Vieira
@ 2020-07-20 13:33 ` Andre Vieira (lists)
0 siblings, 0 replies; 14+ messages in thread
From: Andre Vieira (lists) @ 2020-07-20 13:33 UTC (permalink / raw)
To: gcc-patches, Kyrylo Tkachov
On 08/07/2020 09:04, Andre Simoes Dias Vieira wrote:
>
> On 07/07/2020 13:43, Christophe Lyon wrote:
>> Hi,
>>
>>
>> On Mon, 6 Jul 2020 at 16:31, Andre Vieira (lists)
>> <andre.simoesdiasvieira@arm.com> wrote:
>>>
>>> On 30/06/2020 14:50, Andre Vieira (lists) wrote:
>>>> On 29/06/2020 11:15, Christophe Lyon wrote:
>>>>> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
>>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>>> On 23/06/2020 21:52, Christophe Lyon wrote:
>>>>>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
>>>>>>> <andre.simoesdiasvieira@arm.com> wrote:
>>>>>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
>>>>>>>>>> Sent: 22 June 2020 09:52
>>>>>>>>>> To: gcc-patches@gcc.gnu.org
>>>>>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>>>>>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber
>>>>>>>>>> callee saved
>>>>>>>>>> registers with CMSE
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> As reported in bugzilla when the -mcmse option is used while
>>>>>>>>>> compiling
>>>>>>>>>> for size (-Os) with a thumb-1 target the generated code will
>>>>>>>>>> clear the
>>>>>>>>>> registers r7-r10. These however are callee saved and should be
>>>>>>>>>> preserved
>>>>>>>>>> accross ABI boundaries. The reason this happens is because these
>>>>>>>>>> registers are made "fixed" when optimising for size with Thumb-1
>>>>>>>>>> in a
>>>>>>>>>> way to make sure they are not used, as pushing and popping
>>>>>>>>>> hi-registers
>>>>>>>>>> requires extra moves to and from LO_REGS.
>>>>>>>>>>
>>>>>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which
>>>>>>>>>> accounts for
>>>>>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be
>>>>>>>>>> aware of
>>>>>>>>>> 'callee_saved_reg_p''s definition, as it does still take call
>>>>>>>>>> used
>>>>>>>>>> registers into account, which aren't callee_saved in my opinion,
>>>>>>>>>> so it
>>>>>>>>>> is a rather misnoemer, works in our advantage here though as it
>>>>>>>>>> does
>>>>>>>>>> exactly what we need.
>>>>>>>>>>
>>>>>>>>>> Regression tested on arm-none-eabi.
>>>>>>>>>>
>>>>>>>>>> Is this OK for trunk? (Will eventually backport to previous
>>>>>>>>>> versions if
>>>>>>>>>> stable.)
>>>>>>>>> Ok.
>>>>>>>>> Thanks,
>>>>>>>>> Kyrill
>>>>>>>> As I was getting ready to push this I noticed I didn't add any
>>>>>>>> skip-ifs
>>>>>>>> to prevent this failing with specific target options. So here's
>>>>>>>> a new
>>>>>>>> version with those.
>>>>>>>>
>>>>>>>> Still OK?
>>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> This is not sufficient to skip arm-linux-gnueabi* configs built
>>>>>>> with
>>>>>>> non-default cpu/fpu.
>>>>>>>
>>>>>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
>>>>>>> --with-fpu=neon-fp16 --with-float=hard
>>>>>>> I see:
>>>>>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
>>>>>>> Excess errors:
>>>>>>> cc1: error: ARMv8-M Security Extensions incompatible with
>>>>>>> selected FPU
>>>>>>> cc1: error: target CPU does not support ARM mode
>>>>>>>
>>>>>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
>>>>>> Resending as I don't think my earlier one made it to the lists
>>>>>> (sorry if
>>>>>> you are receiving this double!)
>>>>>>
>>>>>> I'm not following this, before I go off and try to reproduce it,
>>>>>> what do
>>>>>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse
>>>>>> -Os'?
>>>>>> These are the options you are seeing in the log file? Surely they
>>>>>> should
>>>>>> override the default options? Only thing I can think of is this
>>>>>> might
>>>>>> need an extra -mfloat-abi=soft to make sure it overrides the default
>>>>>> float-abi. Could you give that a try?
>>>>> No it doesn't make a difference alone.
>>>>>
>>>>> I also had to add:
>>>>> -mfpu=auto (that clears the above warning)
>>>>> -mthumb otherwise we now get cc1: error: target CPU does not support
>>>>> ARM mode
>>>>>
>>>>> Looks like some effective-target machinery is needed
>>>> So I had a look at this, I was pretty sure that -mfloat-abi=soft
>>>> overwrote -mfpu=<>, which in large it does, as in no FP instructions
>>>> will be generated but the error you see only checks for the right
>>>> number of FP registers. Which doesn't check whether
>>>> 'TARGET_HARD_FLOAT' is set or not. I'll fix this too and use the
>>>> check-effective-target for armv8-m.base for this test as it is indeed
>>>> a better approach than my bag of skip-ifs. I'm testing it locally to
>>>> make sure my changes don't break anything.
>>>>
>>>> Cheers,
>>>> Andre
>>> Hi,
>>>
>>> Sorry for the delay. So I changed the test to use the effective-target
>>> machinery as you suggested and I also made sure that you don't get the
>>> "ARMv8-M Security Extensions incompatible with selected FPU" when
>>> -mfloat-abi=soft.
>>> Further changed 'asm' to '__asm__' to avoid failures with '-std='
>>> options.
>>>
>>> Regression tested on arm-none-eabi.
>>> @Christophe: could you test this for your configuration, shouldn't fail
>>> anymore!
>>>
>> Indeed with your patch I don't see any failure with pr95646.c
>>
>> Note that it is still unsupported with arm-eabi when running the tests
>> with -mcpu=cortex-mXX
>> because the compiler complains that -mcpu=cortex-mXX conflicts with
>> -march=armv8-m.base,
>> thus the effective-target test fails.
>>
>> BTW, is that warning useful/practical? Wouldn't it be more convenient
>> if the last -mcpu/-march
>> on the command line was the only one taken into account? (I had a
>> similar issue when
>> running tests (libstdc++) getting -march=armv8-m.main+fp from their
>> multilib environment
>> and forcing -mcpu=cortex-m33 because it also means '+dsp' and produces
>> a warning;
>> I had to use -mcpu=cortex-m33 -march=armv8-m.main+fp+dsp to
>> workaround this)
> Yeah I've been annoyed by that before, also in the context of testing
> multilibs.
>
> Even though I can see how it can be a useful warning though, if you
> are using these in build-systems and you accidentally introduce a new
> (incompatible) -mcpu/-march alongside the old one. Though it seems
> arbitrary, as we do not warn against multiple -mcpu's or -march's...
>
> I would be in favor of introducing a flag to turn off the warning,
> such that we don't have to worry about it in the testsuite.
> Unfortunately, it seems -march always wins, regardless of order ....
> Meaning that if we do add and use such a warning-off option, we would
> also have to always use '-march' as additional-options when creating
> testcases, to ensure it overrides any RUNTESTFLAG incompatible -mcpu
> or different -march options.
>
> Either way, its a bit of a mess that could probably do with some
> cleaning up. One thing at a time though.
Ping.
Still need an OK to commit this patch.
>>
>> It passes with the config I test without overriding runtestflags flags
>> though, so it's covered.
>>
>> Thanks
>>
>> Christophe
>>
>>> Is this OK for trunk?
>>>
>>> Cheers,
>>> Andre
>>>
>>> gcc/ChangeLog:
>>> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>
>>> * config/arm/arm.c
>>> (arm_options_perform_arch_sanity_checks): Do not
>>> check +D32 for CMSE if -mfloat-abi=soft
>>>
>>> gcc/testsuite/ChangeLog:
>>> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>
>>> * gcc.target/arm/pr95646.c: Fix testism.
>>>>> Christophe
>>>>>
>>>>>
>>>>>> Cheers,
>>>>>> Andre
>>>>>>> Christophe
>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Andre
>>>>>>>>>> Cheers,
>>>>>>>>>> Andre
>>>>>>>>>>
>>>>>>>>>> gcc/ChangeLog:
>>>>>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>>>>
>>>>>>>>>> PR target/95646
>>>>>>>>>> * config/arm/arm.c:
>>>>>>>>>> (cmse_nonsecure_entry_clear_before_return):
>>>>>>>>>> Use 'callee_saved_reg_p' instead of
>>>>>>>>>> 'calL_used_or_fixed_reg_p'.
>>>>>>>>>>
>>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
>>>>>>>>>>
>>>>>>>>>> PR target/95646
>>>>>>>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE
2020-07-06 14:30 ` Andre Vieira (lists)
2020-07-07 12:43 ` Christophe Lyon
@ 2020-08-06 9:56 ` Kyrylo Tkachov
1 sibling, 0 replies; 14+ messages in thread
From: Kyrylo Tkachov @ 2020-08-06 9:56 UTC (permalink / raw)
To: Andre Simoes Dias Vieira, gcc-patches, Christophe Lyon
Hi Andre,
> -----Original Message-----
> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> Sent: 06 July 2020 15:31
> To: gcc-patches@gcc.gnu.org; Christophe Lyon <christophe.lyon@linaro.org>
> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Subject: Re: [PATCH][GCC][Arm] PR target/95646: Do not clobber callee
> saved registers with CMSE
>
>
> On 30/06/2020 14:50, Andre Vieira (lists) wrote:
> >
> > On 29/06/2020 11:15, Christophe Lyon wrote:
> >> On Mon, 29 Jun 2020 at 10:56, Andre Vieira (lists)
> >> <andre.simoesdiasvieira@arm.com> wrote:
> >>>
> >>> On 23/06/2020 21:52, Christophe Lyon wrote:
> >>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists)
> >>>> <andre.simoesdiasvieira@arm.com> wrote:
> >>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvieira@arm.com>
> >>>>>>> Sent: 22 June 2020 09:52
> >>>>>>> To: gcc-patches@gcc.gnu.org
> >>>>>>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> >>>>>>> Subject: [PATCH][GCC][Arm] PR target/95646: Do not clobber
> >>>>>>> callee saved
> >>>>>>> registers with CMSE
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> As reported in bugzilla when the -mcmse option is used while
> >>>>>>> compiling
> >>>>>>> for size (-Os) with a thumb-1 target the generated code will
> >>>>>>> clear the
> >>>>>>> registers r7-r10. These however are callee saved and should be
> >>>>>>> preserved
> >>>>>>> accross ABI boundaries. The reason this happens is because these
> >>>>>>> registers are made "fixed" when optimising for size with Thumb-1
> >>>>>>> in a
> >>>>>>> way to make sure they are not used, as pushing and popping
> >>>>>>> hi-registers
> >>>>>>> requires extra moves to and from LO_REGS.
> >>>>>>>
> >>>>>>> To fix this, this patch uses 'callee_saved_reg_p', which
> >>>>>>> accounts for
> >>>>>>> this optimisation, instead of 'call_used_or_fixed_reg_p'. Be
> >>>>>>> aware of
> >>>>>>> 'callee_saved_reg_p''s definition, as it does still take call used
> >>>>>>> registers into account, which aren't callee_saved in my opinion,
> >>>>>>> so it
> >>>>>>> is a rather misnoemer, works in our advantage here though as it
> >>>>>>> does
> >>>>>>> exactly what we need.
> >>>>>>>
> >>>>>>> Regression tested on arm-none-eabi.
> >>>>>>>
> >>>>>>> Is this OK for trunk? (Will eventually backport to previous
> >>>>>>> versions if
> >>>>>>> stable.)
> >>>>>> Ok.
> >>>>>> Thanks,
> >>>>>> Kyrill
> >>>>> As I was getting ready to push this I noticed I didn't add any
> >>>>> skip-ifs
> >>>>> to prevent this failing with specific target options. So here's a new
> >>>>> version with those.
> >>>>>
> >>>>> Still OK?
> >>>>>
> >>>> Hi,
> >>>>
> >>>> This is not sufficient to skip arm-linux-gnueabi* configs built with
> >>>> non-default cpu/fpu.
> >>>>
> >>>> For instance, with arm-linux-gnueabihf --with-cpu=cortex-a9
> >>>> --with-fpu=neon-fp16 --with-float=hard
> >>>> I see:
> >>>> FAIL: gcc.target/arm/pr95646.c (test for excess errors)
> >>>> Excess errors:
> >>>> cc1: error: ARMv8-M Security Extensions incompatible with selected
> FPU
> >>>> cc1: error: target CPU does not support ARM mode
> >>>>
> >>>> and the testcase is compiled with -mcpu=cortex-m23 -mcmse -Os
> >>> Resending as I don't think my earlier one made it to the lists
> >>> (sorry if
> >>> you are receiving this double!)
> >>>
> >>> I'm not following this, before I go off and try to reproduce it,
> >>> what do
> >>> you mean by 'the testcase is compiled with -mcpu=cortex-m23 -mcmse
> >>> -Os'?
> >>> These are the options you are seeing in the log file? Surely they
> >>> should
> >>> override the default options? Only thing I can think of is this might
> >>> need an extra -mfloat-abi=soft to make sure it overrides the default
> >>> float-abi. Could you give that a try?
> >> No it doesn't make a difference alone.
> >>
> >> I also had to add:
> >> -mfpu=auto (that clears the above warning)
> >> -mthumb otherwise we now get cc1: error: target CPU does not support
> >> ARM mode
> >>
> >> Looks like some effective-target machinery is needed
> > So I had a look at this, I was pretty sure that -mfloat-abi=soft
> > overwrote -mfpu=<>, which in large it does, as in no FP instructions
> > will be generated but the error you see only checks for the right
> > number of FP registers. Which doesn't check whether
> > 'TARGET_HARD_FLOAT' is set or not. I'll fix this too and use the
> > check-effective-target for armv8-m.base for this test as it is indeed
> > a better approach than my bag of skip-ifs. I'm testing it locally to
> > make sure my changes don't break anything.
> >
> > Cheers,
> > Andre
> Hi,
>
> Sorry for the delay. So I changed the test to use the effective-target
> machinery as you suggested and I also made sure that you don't get the
> "ARMv8-M Security Extensions incompatible with selected FPU" when
> -mfloat-abi=soft.
> Further changed 'asm' to '__asm__' to avoid failures with '-std=' options.
>
> Regression tested on arm-none-eabi.
> @Christophe: could you test this for your configuration, shouldn't fail
> anymore!
>
> Is this OK for trunk?
Sorry for the delay, this is ok.
Thanks,
Kyrill
>
> Cheers,
> Andre
>
> gcc/ChangeLog:
> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> * config/arm/arm.c (arm_options_perform_arch_sanity_checks): Do
> not
> check +D32 for CMSE if -mfloat-abi=soft
Full stop in ChangeLog etc.
>
> gcc/testsuite/ChangeLog:
> 2020-07-06 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
> * gcc.target/arm/pr95646.c: Fix testism.
> >>
> >> Christophe
> >>
> >>
> >>> Cheers,
> >>> Andre
> >>>> Christophe
> >>>>
> >>>>> Cheers,
> >>>>> Andre
> >>>>>>> Cheers,
> >>>>>>> Andre
> >>>>>>>
> >>>>>>> gcc/ChangeLog:
> >>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>>>>>>
> >>>>>>> PR target/95646
> >>>>>>> * config/arm/arm.c:
> >>>>>>> (cmse_nonsecure_entry_clear_before_return):
> >>>>>>> Use 'callee_saved_reg_p' instead of
> >>>>>>> 'calL_used_or_fixed_reg_p'.
> >>>>>>>
> >>>>>>> gcc/testsuite/ChangeLog:
> >>>>>>> 2020-06-22 Andre Vieira <andre.simoesdiasvieira@arm.com>
> >>>>>>>
> >>>>>>> PR target/95646
> >>>>>>> * gcc.target/arm/pr95646.c: New test.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-08-06 9:56 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22 8:51 [PATCH][GCC][Arm] PR target/95646: Do not clobber callee saved registers with CMSE Andre Vieira (lists)
2020-06-23 12:10 ` Kyrylo Tkachov
2020-06-23 13:28 ` Andre Vieira (lists)
2020-06-23 13:29 ` Kyrylo Tkachov
2020-06-23 20:52 ` Christophe Lyon
2020-06-29 8:45 ` Andre Vieira (lists)
2020-06-29 8:56 ` Andre Vieira (lists)
2020-06-29 10:15 ` Christophe Lyon
2020-06-30 13:50 ` Andre Vieira (lists)
2020-07-06 14:30 ` Andre Vieira (lists)
2020-07-07 12:43 ` Christophe Lyon
2020-07-08 8:04 ` Andre Simoes Dias Vieira
2020-07-20 13:33 ` Andre Vieira (lists)
2020-08-06 9:56 ` Kyrylo Tkachov
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).