Re: [GCC][PATCH v2] arm: Add cde feature support for Cortex-M55 CPU.

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 31/10/2022 12:38, Srinath Parvathaneni via Gcc-patches wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Christophe Lyon <Christophe.Lyon@arm.com>
>> Sent: Monday, October 17, 2022 2:30 PM
>> To: Srinath Parvathaneni <Srinath.Parvathaneni@arm.com>; gcc-
>> patches@gcc.gnu.org
>> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>
>> Subject: Re: [GCC][PATCH] arm: Add cde feature support for Cortex-M55
>> CPU.
>>
>> Hi Srinath,
>>
>>
>> On 10/10/22 10:20, Srinath Parvathaneni via Gcc-patches wrote:
>>> Hi,
>>>
>>> This patch adds cde feature (optional) support for Cortex-M55 CPU,
>>> please refer [1] for more details. To use this feature we need to
>>> specify +cdecpN (e.g. -mcpu=cortex-m55+cdecp<N>), where N is the
>> coprocessor number 0 to 7.
>>>
>>> Bootstrapped for arm-none-linux-gnueabihf target, regression tested on
>>> arm-none-eabi target and found no regressions.
>>>
>>> [1] https://developer.arm.com/documentation/101051/0101/?lang=en
>> (version: r1p1).
>>>
>>> Ok for master?
>>>
>>> Regards,
>>> Srinath.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2022-10-07  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
>>>
>>>           * common/config/arm/arm-common.cc (arm_canon_arch_option_1):
>> Ignore cde
>>>           options for mlibarch.
>>>           * config/arm/arm-cpus.in (begin cpu cortex-m55): Add cde options.
>>>           * doc/invoke.texi (CDE): Document options for Cortex-M55 CPU.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2022-10-07  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
>>>
>>>           * gcc.target/arm/multilib.exp: Add multilib tests for Cortex-M55 CPU.
>>>
>>>
>>> ###############     Attachment also inlined for ease of reply
>> ###############
>>>
>>>
>>> diff --git a/gcc/common/config/arm/arm-common.cc
>>> b/gcc/common/config/arm/arm-common.cc
>>> index
>>>
>> c38812f1ea6a690cd19b0dc74d963c4f5ae155ca..b6f955b3c012475f398382e72
>> c9a
>>> 3966412991ec 100644
>>> --- a/gcc/common/config/arm/arm-common.cc
>>> +++ b/gcc/common/config/arm/arm-common.cc
>>> @@ -753,6 +753,15 @@ arm_canon_arch_option_1 (int argc, const char
>> **argv, bool arch_for_multilib)
>>>          arm_initialize_isa (target_isa, selected_cpu->common.isa_bits);
>>>          arm_parse_option_features (target_isa, &selected_cpu->common,
>>>    				 strchr (cpu, '+'));
>>> +      if (arch_for_multilib)
>>> +	{
>>> +	  const enum isa_feature removable_bits[] =
>> {ISA_IGNORE_FOR_MULTILIB,
>>> +						     isa_nobit};
>>> +	  sbitmap isa_bits = sbitmap_alloc (isa_num_bits);
>>> +	  arm_initialize_isa (isa_bits, removable_bits);
>>> +	  bitmap_and_compl (target_isa, target_isa, isa_bits);
>>> +	}
>>> +
>>
>> I can see the piece of code you add here is exactly the same as the one a few
>> lines above when handling "if (arch)". Can this be moved below and thus be
>> common to the two cases, or does it have to be performed before
>> bitmap_ior of fpu_isa?
> 
> Thanks for pointing out this, I have moved the common code below the arch and cpu
> if blocks in the attached patch.
>   
>> Also, IIUC, CDE was already optional for other CPUs (M33, M35P, star-mc1),
>> so the hunk above fixes a latent bug when handling multilibs for these CPUs
>> too? If so, maybe worth splitting the patch into two parts since the above is
>> not strictly related to M55?
>>
> Even though CDE is optional for the mentioned CPUs as per the specs, the code to
> enable CDE as optional feature is missing in current compiler.
> Current GCC compiler supports CDE as optional feature only with -march options and
> this pass adds CDE as optional for M55 and so this is not a fix bug.
> 
>> But I'm not a maintainer ;-)
>>
>> Thanks,
>>
>> Christophe
>>
>>>          if (fpu && strcmp (fpu, "auto") != 0)
>>>    	{
>>>    	  /* The easiest and safest way to remove the default fpu diff
>>> --git a/gcc/config/arm/arm-cpus.in b/gcc/config/arm/arm-cpus.in index
>>>
>> 5a63bc548e54dbfdce5d1df425bd615d81895d80..aa02c04c4924662f3ddd58e
>> 69673
>>> 92ba3f4b4a87 100644
>>> --- a/gcc/config/arm/arm-cpus.in
>>> +++ b/gcc/config/arm/arm-cpus.in
>>> @@ -1633,6 +1633,14 @@ begin cpu cortex-m55
>>>     option nomve remove mve mve_float
>>>     option nofp remove ALL_FP mve_float
>>>     option nodsp remove MVE mve_float
>>> + option cdecp0 add cdecp0
>>> + option cdecp1 add cdecp1
>>> + option cdecp2 add cdecp2
>>> + option cdecp3 add cdecp3
>>> + option cdecp4 add cdecp4
>>> + option cdecp5 add cdecp5
>>> + option cdecp6 add cdecp6
>>> + option cdecp7 add cdecp7
>>>     isa quirk_no_asmcpu quirk_vlldm
>>>     costs v7m
>>>     vendor 41
>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index
>>>
>> aa5655764a0360959f9c1061749d2cc9ebd23489..26857f7a90e42d925bc69086
>> 86ac
>>> 78138a53c4ad 100644
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -21698,6 +21698,10 @@ floating-point instructions on @samp{cortex-
>> m55}.
>>>    Disable the M-Profile Vector Extension (MVE) single precision floating-
>> point
>>>    instructions on @samp{cortex-m55}.
>>>
>>> +@item +cdecp0, +cdecp1, ... , +cdecp7 Enable the Custom Datapath
>>> +Extension (CDE) on selected coprocessors according to the numbers
>>> +given in the options in the range 0 to 7 on @samp{cortex-m55}.
>>> +
>>>    @item  +nofp
>>>    Disables the floating-point instructions on @samp{arm9e},
>>>    @samp{arm946e-s}, @samp{arm966e-s}, @samp{arm968e-s},
>> @samp{arm10e},
>>> diff --git a/gcc/testsuite/gcc.target/arm/multilib.exp
>>> b/gcc/testsuite/gcc.target/arm/multilib.exp
>>> index
>>>
>> 2fa648c61dafebb663969198bf7849400a7547f6..7a977bff58b7b68bfe9e49d7
>> 6029
>>> 89a39caa6534 100644
>>> --- a/gcc/testsuite/gcc.target/arm/multilib.exp
>>> +++ b/gcc/testsuite/gcc.target/arm/multilib.exp
>>> @@ -851,6 +851,18 @@ if {[multilib_config "rmprofile"] } {
>>>    	{-mcpu=cortex-m55+nomve+nofp -mfpu=auto -mfloat-abi=softfp}
>> "thumb/v8-m.main/nofp"
>>>    	{-mcpu=cortex-m55+nodsp+nofp -mfpu=auto -mfloat-abi=soft}
>> "thumb/v8-m.main/nofp"
>>>    	{-mcpu=cortex-m55+nodsp+nofp -mfpu=auto -mfloat-abi=softfp}
>> "thumb/v8-m.main/nofp"
>>> +	{-mcpu=cortex-m55 -mfloat-abi=hard -mfpu=auto} "thumb/v8-
>> m.main+dp/hard"
>>> +	{-mcpu=cortex-m55+cdecp0 -mfloat-abi=hard -mfpu=auto}
>> "thumb/v8-m.main+dp/hard"
>>> +	{-mcpu=cortex-m55+nomve+cdecp0 -mfloat-abi=hard -mfpu=auto}
>> "thumb/v8-m.main+dp/hard"
>>> +	{-mcpu=cortex-
>> m55+cdecp0+cdecp1+cdecp2+cdecp3+cdecp4+cdecp5+cdecp6+cdecp7 -
>> mfloat-abi=hard -mfpu=auto} "thumb/v8-m.main+dp/hard"
>>> +	{-mcpu=cortex-m55 -mfloat-abi=softfp -mfpu=auto} "thumb/v8-
>> m.main+dp/softfp"
>>> +	{-mcpu=cortex-m55+cdecp0 -mfloat-abi=softfp -mfpu=auto}
>> "thumb/v8-m.main+dp/softfp"
>>> +	{-mcpu=cortex-m55+nomve+cdecp0 -mfloat-abi=softfp -mfpu=auto}
>> "thumb/v8-m.main+dp/softfp"
>>> +	{-mcpu=cortex-
>> m55+cdecp0+cdecp1+cdecp2+cdecp3+cdecp4+cdecp5+cdecp6+cdecp7 -
>> mfloat-abi=softfp -mfpu=auto} "thumb/v8-m.main+dp/softfp"
>>> +	{-mcpu=cortex-m55 -mfloat-abi=soft -mfpu=auto} "thumb/v8-
>> m.main/nofp"
>>> +	{-mcpu=cortex-m55+cdecp0 -mfloat-abi=soft -mfpu=auto}
>> "thumb/v8-m.main/nofp"
>>> +	{-mcpu=cortex-m55+nomve+cdecp0 -mfloat-abi=soft -mfpu=auto}
>> "thumb/v8-m.main/nofp"
>>> +	{-mcpu=cortex-
>> m55+cdecp0+cdecp1+cdecp2+cdecp3+cdecp4+cdecp5+cdecp6+cdecp7 -
>> mfloat-abi=soft -mfpu=auto} "thumb/v8-m.main/nofp"
>>>    	{-march=armv8-m.main+cdecp0 -mfpu=auto -mfloat-abi=soft}
>> "thumb/v8-m.main/nofp"
>>>    	{-march=armv8-m.main+fp+cdecp0 -mfpu=auto -mfloat-abi=soft}
>> "thumb/v8-m.main/nofp"
>>>    	{-march=armv8-m.main+fp.dp+cdecp0 -mfpu=auto -mfloat-abi=soft}
>> "thumb/v8-m.main/nofp"
>>>
>>>
>>>

+  auto_sbitmap removable_isa (isa_num_bits);

Removable isn't a good description of this variable.  I think it would 
be better to use something closer to the ISA_IGNORE_FOR_MULTILIB macro 
that gets defined from arm-cpus.in:  perhaps 'ignore_multilib_isa'.

OK with that change.

R.