Hi Andre, > -----Original Message----- > From: Andre Vieira (lists) <andre.simoesdiasvie...@arm.com> > Sent: 06 July 2020 15:31 > To: gcc-patches@gcc.gnu.org; Christophe Lyon <christophe.l...@linaro.org> > Cc: Kyrylo Tkachov <kyrylo.tkac...@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.simoesdiasvie...@arm.com> wrote: > >>> > >>> On 23/06/2020 21:52, Christophe Lyon wrote: > >>>> On Tue, 23 Jun 2020 at 15:28, Andre Vieira (lists) > >>>> <andre.simoesdiasvie...@arm.com> wrote: > >>>>> On 23/06/2020 13:10, Kyrylo Tkachov wrote: > >>>>>>> -----Original Message----- > >>>>>>> From: Andre Vieira (lists) <andre.simoesdiasvie...@arm.com> > >>>>>>> Sent: 22 June 2020 09:52 > >>>>>>> To: gcc-patches@gcc.gnu.org > >>>>>>> Cc: Kyrylo Tkachov <kyrylo.tkac...@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.simoesdiasvie...@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.simoesdiasvie...@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.simoesdiasvie...@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.simoesdiasvie...@arm.com> > >>>>>>> > >>>>>>> PR target/95646 > >>>>>>> * gcc.target/arm/pr95646.c: New test.
