Re: [PATCH][ARM][PING] __ARM_FP & __ARM_NEON_FP defined when -march=armv7-m

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 3 February 2015 at 17:29, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 06/01/15 09:40, Mantas Mikaitis wrote:
>>
>> Ping and changelog spaces removed.
>>
>> Thank you,
>> Mantas M.
>>
>> On 18/11/14 11:58, Richard Earnshaw wrote:
>>> On 18/11/14 11:30, Mantas Mikaitis wrote:
>>>> Incorrect predefinitions for certain target architectures. E.g. arm7-m
>>>> does not contain NEON but the defintion __ARM_NEON_FP was switched on.
>>>> Similarly with armv6 and even armv2.
>>>>
>>>> This patch fixes the predefines for each of the different chips
>>>> containing certain types of the FPU implementations.
>>>>
>>>> Tests:
>>>>
>>>> Tested on arm-none-linux-gnueabi and arm-none-linux-gnueabihf without
>>>> any new regression.
>>>>
>>>> Manually compiled for various targets and all correct definitions were
>>>> present.
>>>>
>>>> Is this patch ok for trunk?
>>>>
>>>> Mantas
>>>>
>>>> gcc/Changelog:
>>>>
>>>>        * config/arm/arm.h (TARGET_NEON_FP): Removed conditional definition, define to zero if !TARGET_NEON.
>>>>          (TARGET_CPU_CPP_BUILTINS): Added second condition before defining __ARM_FP macro.
>>>>
>>>>
>>>> ARM_DEFS.patch
>>>>
>>>>
>>>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>>>> index ff4ddac..325fea9 100644
>>>> --- a/gcc/config/arm/arm.h
>>>> +++ b/gcc/config/arm/arm.h
>>>> @@ -118,7 +118,7 @@ extern char arm_arch_name[];
>>>>     if (TARGET_VFP)                                 \
>>>>       builtin_define ("__VFP_FP__");                \
>>>>                                                     \
>>>> -   if (TARGET_ARM_FP)                              \
>>>> +   if (TARGET_ARM_FP && !TARGET_SOFT_FLOAT)        \
>>> Wouldn't it be better to factor this into TARGET_ARM_FP?  It seems odd
>>> that that macro returns a set of values based on something completely
>>> unavailable for the current compilation.  That would also then mirror
>>> the behaviour of TARGET_NEON_FP (see below) and make the internal macros
>>> more consistent.
>>>
>>> R.
>>
>> Thank you. Patch updated.
>>
>> Ok for trunk?
>>
>> Mantas M.
>>
>> gcc/Changelog
>>
>> 2014-12-03  Mantas Mikaits  <Mantas.Mikaitis@arm.com>
>>
>>            * config/arm/arm.h (TARGET_NEON_FP): Removed conditional definition, define to zero if !TARGET_NEON.
>>              (TARGET_ARM_FP): Added !TARGET_SOFT_FLOAT into the conditional definition.
>>
>> gcc/testsuite/ChangeLog:
>>
>>            * gcc.target/arm/macro_defs0.c: New test.
>>            * gcc.target/arm/macro_defs1.c: New test.
>>            * gcc.target/arm/macro_defs2.c: New test.
>>
>>
>
> OK.  However, watch your ChangeLog line length (80 char limit).  Also,
> entries (even continuation lines) should be indented with exactly one
> (hard) tab.
>
> R.
>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> mypatch.patch
>>
>>
>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>> index ff4ddac..7d4cc39 100644
>> --- a/gcc/config/arm/arm.h
>> +++ b/gcc/config/arm/arm.h
>> @@ -2343,17 +2343,17 @@ extern int making_const_table;
>>     point types.  Where bit 1 indicates 16-bit support, bit 2 indicates
>>     32-bit support, bit 3 indicates 64-bit support.  */
>>  #define TARGET_ARM_FP                        \
>> -  (TARGET_VFP_SINGLE ? 4             \
>> -                  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0))
>> +  (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4               \
>> +                     : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>> +                   : 0)
>>
>>
>>  /* Set as a bit mask indicating the available widths of floating point
>>     types for hardware NEON floating point.  This is the same as
>>     TARGET_ARM_FP without the 64-bit bit set.  */
>> -#ifdef TARGET_NEON
>> -#define TARGET_NEON_FP               \
>> -  (TARGET_ARM_FP & (0xff ^ 0x08))
>> -#endif
>> +#define TARGET_NEON_FP                                \
>> +  (TARGET_NEON ? (TARGET_ARM_FP & (0xff ^ 0x08)) \
>> +            : 0)
>>
>>  /* The maximum number of parallel loads or stores we support in an ldm/stm
>>     instruction.  */
>> diff --git a/gcc/testsuite/gcc.target/arm/macro_defs0.c b/gcc/testsuite/gcc.target/arm/macro_defs0.c
>> new file mode 100644
>> index 0000000..198243e
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/macro_defs0.c
>> @@ -0,0 +1,14 @@
>> +/* { dg-do compile } */
>> +/* { dg-skip-if "avoid conflicting multilib options"
>> +   { *-*-* } { "-march=*" } {"-march=armv7-m"} } */
>> +/* { dg-skip-if "avoid conflicting multilib options"
>> +   { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=soft" } } */
>> +/* { dg-options "-march=armv7-m -mcpu=cortex-m3 -mfloat-abi=soft -mthumb" } */
>> +
>> +#ifdef __ARM_FP
>> +#error __ARM_FP should not be defined
>> +#endif
>> +
>> +#ifdef __ARM_NEON_FP
>> +#error __ARM_NEON_FP should not be defined
>> +#endif
>> diff --git a/gcc/testsuite/gcc.target/arm/macro_defs1.c b/gcc/testsuite/gcc.target/arm/macro_defs1.c
>> new file mode 100644
>> index 0000000..075b71b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/macro_defs1.c
>> @@ -0,0 +1,9 @@
>> +/* { dg-do compile } */
>> +/* { dg-skip-if "avoid conflicting multilib options"
>> +   { *-*-* } { "-march=*" } { "-march=armv6-m" } } */
>> +/* { dg-options "-march=armv6-m -mthumb" } */

Minor comment:
You are probably aware of that, but in a multilib which forces '-marm'
this test fails because:
testsuite/gcc.target/arm/macro_defs1.c:1:0: error: target CPU does not
support ARM mode
(as a result of compiling it with -march=armv6-m -mthumb -marm)

Christophe

>> +
>> +#ifdef __ARM_NEON_FP
>> +#error __ARM_NEON_FP should not be defined
>> +#endif
>> +
>> diff --git a/gcc/testsuite/gcc.target/arm/macro_defs2.c b/gcc/testsuite/gcc.target/arm/macro_defs2.c
>> new file mode 100644
>> index 0000000..9a96042
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/macro_defs2.c
>> @@ -0,0 +1,14 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-march=armv7ve -mcpu=cortex-a15 -mfpu=neon-vfpv4" } */
>> +/* { dg-add-options arm_neon } */
>> +/* { dg-require-effective-target arm_neon_ok } */
>> +
>> +#ifndef __ARM_NEON_FP
>> +#error  __ARM_NEON_FP is not defined but should be
>> +#endif
>> +
>> +#ifndef __ARM_FP
>> +#error  __ARM_FP is not defined but should be
>> +#endif
>> +
>> +
>>
>
>