Re: [PATCH, ARM] Do not set ARM_ARCH_ISA_THUMB for armv5

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Thomas,

On 10/05/16 14:26, Thomas Preudhomme wrote:
> Hi,
>
> ARM_ARCH_ISA_THUMB is currently set to 1 when compiling for armv5 despite
> armv5 not supporting Thumb instructions (armv5t does):
>
> arm-none-eabi-gcc -dM -march=armv5 -E - < /dev/null | grep ISA_THUMB
> #define __ARM_ARCH_ISA_THUMB 1
>
> The reason is TARGET_ARM_ARCH_ISA_THUMB being set to 1 if target does not
> support Thumb-2 and is ARMv4T, ARMv5 or later. This patch replaces that logic
> by checking whether the given architecture has the right feature bit
> (FL_THUMB).
>
> ChangeLog entry is as follows:
>
>
> *** gcc/ChangeLog ***
>
> 2016-05-06  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>
>          * config/arm/arm-protos.h (arm_arch_thumb): Declare.
>          * config/arm/arm.c (arm_arch_thumb): Define.
>          (arm_option_override): Initialize arm_arch_thumb.
>          * config/arm/arm.h (TARGET_ARM_ARCH_ISA_THUMB): Use arm_arch_thumb to
>          determine if target support Thumb-1 ISA.
>
>
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index
> d8179c441bb53dced94d2ebf497aad093e4ac600..4d11c91133ff1b875afcbf58abc4491c2c93768e
> 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -603,6 +603,9 @@ extern int arm_tune_cortex_a9;
>      interworking clean.  */
>   extern int arm_cpp_interwork;
>   
> +/* Nonzero if chip supports Thumb.  */
> +extern int arm_arch_thumb;
> +

Bit of bikeshedding really, but I think a better name would be
arm_arch_thumb1.
This is because we also have the macros TARGET_THUMB and TARGET_THUMB2
where TARGET_THUMB2 means either Thumb-1 or Thumb-2 and a casual reader
might think that arm_arch_thumb means that there is support for either.

Also, please add a simple test that compiles something with -march=armv5 (plus -marm)
and checks that __ARM_ARCH_ISA_THUMB is not defined.

Ok with that change and the test.

Thanks,
Kyrill

P.S. I think your mailer sometimes mangles long lines in the patches
(for example the git hash headers). Can you please send your patches as
attachments? That will also make it easier for me to extract and apply
them to my tree without having to manually select the inlined patch
from the message.

>   /* Nonzero if chip supports Thumb 2.  */
>   extern int arm_arch_thumb2;
>   
> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
> index
> ad123dde991a3e4c4b9563ee6ebb84981767988f..f64e8caa8bc08b7aff9fe385567de9936a964004
> 100644
> --- a/gcc/config/arm/arm.h
> +++ b/gcc/config/arm/arm.h
> @@ -2191,9 +2191,8 @@ extern int making_const_table;
>   #define TARGET_ARM_V7M (!arm_arch_notm && arm_arch_thumb2)
>   
>   /* The highest Thumb instruction set version supported by the chip.  */
> -#define TARGET_ARM_ARCH_ISA_THUMB 		\
> -  (arm_arch_thumb2 ? 2				\
> -	           : ((TARGET_ARM_ARCH >= 5 || arm_arch4t) ? 1 : 0))
> +#define TARGET_ARM_ARCH_ISA_THUMB		\
> +  (arm_arch_thumb2 ? 2 : (arm_arch_thumb ? 1 : 0))
>   
>   /* Expands to an upper-case char of the target's architectural
>      profile.  */
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index
> 71b51439dc7ba5be67671e9fb4c3f18040cce58f..de1c2d4600529518a92ed44815cff05308baa31c
> 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -852,6 +852,9 @@ int arm_tune_cortex_a9 = 0;
>      interworking clean.  */
>   int arm_cpp_interwork = 0;
>   
> +/* Nonzero if chip supports Thumb.  */
> +int arm_arch_thumb;
> +
>   /* Nonzero if chip supports Thumb 2.  */
>   int arm_arch_thumb2;
>   
> @@ -3170,6 +3173,7 @@ arm_option_override (void)
>     arm_arch7em = ARM_FSET_HAS_CPU1 (insn_flags, FL_ARCH7EM);
>     arm_arch8 = ARM_FSET_HAS_CPU1 (insn_flags, FL_ARCH8);
>     arm_arch8_1 = ARM_FSET_HAS_CPU2 (insn_flags, FL2_ARCH8_1);
> +  arm_arch_thumb = ARM_FSET_HAS_CPU1 (insn_flags, FL_THUMB);
>     arm_arch_thumb2 = ARM_FSET_HAS_CPU1 (insn_flags, FL_THUMB2);
>     arm_arch_xscale = ARM_FSET_HAS_CPU1 (insn_flags, FL_XSCALE);
>   
>
>
> Before patch:
>
> % arm-none-eabi-gcc -dM -march=armv4 -E - < /dev/null | grep ISA_THUMB
> cc1: warning: target CPU does not support THUMB instructions
> % arm-none-eabi-gcc -dM -march=armv4t -E - < /dev/null | grep ISA_THUMB
> #define __ARM_ARCH_ISA_THUMB 1
> % arm-none-eabi-gcc -dM -march=armv5 -E - < /dev/null | grep ISA_THUMB
> cc1: warning: target CPU does not support THUMB instructions
> #define __ARM_ARCH_ISA_THUMB 1
> % arm-none-eabi-gcc -dM -march=armv5t -E - < /dev/null | grep ISA_THUMB
> #define __ARM_ARCH_ISA_THUMB 1
>
> After patch:
>
> % arm-none-eabi-gcc -dM -march=armv5 -E - < /dev/null | grep ISA_THUMB
> cc1: warning: target CPU does not support THUMB instructions
> % arm-none-eabi-gcc -dM -march=armv5t -E - < /dev/null | grep ISA_THUMB
> #define __ARM_ARCH_ISA_THUMB 1
> % arm-none-eabi-gcc -dM -march=armv4 -E - < /dev/null | grep ISA_THUMB
> cc1: warning: target CPU does not support THUMB instructions
> % arm-none-eabi-gcc -dM -march=armv4t -E - < /dev/null | grep ISA_THUMB
> #define __ARM_ARCH_ISA_THUMB 1