[PATCH][ARM]Automatically add -mthumb for thumb-only target when mode isn't specified
Maxim Kuvyrkov
maxim.kuvyrkov@linaro.org
Wed Mar 4 08:16:00 GMT 2015
> On Mar 4, 2015, at 5:46 AM, Terry Guo <flameroc@gmail.com> wrote:
>
> On Wed, Mar 4, 2015 at 10:44 AM, Terry Guo <flameroc@gmail.com> wrote:
>> On Mon, Mar 2, 2015 at 9:08 PM, Maxim Kuvyrkov
>> <maxim.kuvyrkov@linaro.org> wrote:
>>>> On Mar 2, 2015, at 4:44 AM, Terry Guo <terry.guo@arm.com> wrote:
>>>>
>>>> Hi there,
>>>>
>>>> If target mode isn't specified via either gcc configuration option
>>>> --with-mode or command line, this patch intends to improve gcc driver to
>>>> automatically add option -mthumb for thumb-only target. Tested with gcc
>>>> regression test for various arm targets, no regression. Is it OK?
>>>>
>>>> BR,
>>>> Terry
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2015-03-02 Terry Guo <terry.guo@arm.com>
>>>>
>>>> * common/config/arm/arm-common.c (arm_is_target_thumb_only): New
>>>> function.
>>>> * config/arm/arm-protos.h (FL_ Macros): Move to ...
>>>> * config/arm/arm-opts.h (FL_ Macros): ... here.
>>>> (struct arm_arch_core_flag): New struct.
>>>> (arm_arch_core_flags): New array for arch/core and flag map.
>>>> * config/arm/arm.h (MODE_SET_SPEC_FUNCTIONS): Define new SPEC
>>>> function.
>>>> (EXTRA_SPEC_FUNCTIONS): Include new SPEC function.
>>>> (MODE_SET_SPECS): New SPEC.
>>>> (DRIVER_SELF_SPECS): Include new SPEC.<gcc-mthumb-option-v5.txt>
>>>
>>> Did you consider approach of implementing this purely inside cc1 rather than driver?
>>>
>>> We do not seem to need to pass -mthumb to assembler or linker since those will pick up ARM-ness / Thumb-ness from function annotations. Therefore we need to handle -marm / -mthumb for cc1 only. What am I missing?
>>>
>>> Also, what's the significance of moving FL_* flags to arm-opts.h? If you had to separate FL_* definitions from the rest of arm-protos.h, then a new dedicated file (e.g., arm-fl.h) would be a better choice for new home of FL_* definitions.
>>>
>>
>> Please find my answers in another email. The attached patch tries to
>> follow your idea that puts those FL_* into separate file named
>> arm-flags.h. Does it look good to you?
Thanks Terry (and everyone else) for explaining why we want to do this in the driver. The substance of the patch looks good to me, and below are some comments and nit-picks. (Also, I'm not an ARM maintainer, so this is a review, not an approval to commit).
Please make sure to update changelog before committing.
> diff --git a/gcc/common/config/arm/arm-common.c b/gcc/common/config/arm/arm-common.c
> index 86673b7..e17ee03 100644
> --- a/gcc/common/config/arm/arm-common.c
> +++ b/gcc/common/config/arm/arm-common.c
> @@ -97,6 +97,28 @@ arm_rewrite_mcpu (int argc, const char **argv)
> return arm_rewrite_selected_cpu (argv[argc - 1]);
> }
>
> +/* Called by driver to check whether the target denoted by current
> + command line options is thumb-only target. If -march present,
> + check the last -march option. If no -march, check the last -mcpu
> + option. */
> +const char *
> +arm_is_target_thumb_only (int argc, const char **argv)
Consider renaming this to arm_target_thumb_only. The "is" part of the name make it sound like a predicate function, which it is not.
> +{
> + unsigned int opt;
> +
> + if (argc)
> + {
> + for (opt = 0; opt < (ARRAY_SIZE (arm_arch_core_flags) - 1); opt++)
> + if ((strcmp (argv[argc - 1], arm_arch_core_flags[opt].name) == 0)
> + && ((arm_arch_core_flags[opt].flags & FL_NOTM) == 0))
> + return "-mthumb";
> +
> + return NULL;
> + }
> + else
> + return NULL;
> +}
> +
> #undef ARM_CPU_NAME_LENGTH
>
>
> diff --git a/gcc/config/arm/arm-flags.h b/gcc/config/arm/arm-flags.h
> new file mode 100644
> index 0000000..fe3a723
> --- /dev/null
> +++ b/gcc/config/arm/arm-flags.h
> @@ -0,0 +1,92 @@
> +/* Flags used to identify the presence of processor capabilities.
> +
> + Copyright (C) 2015 Free Software Foundation, Inc.
> + Contributed by ARM Ltd.
> +
> + This file is part of GCC.
> +
> + GCC is free software; you can redistribute it and/or modify it
> + under the terms of the GNU General Public License as published
> + by the Free Software Foundation; either version 3, or (at your
> + option) any later version.
> +
> + GCC is distributed in the hope that it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> + License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with GCC; see the file COPYING3. If not see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef GCC_ARM_FLAGS_H
> +#define GCC_ARM_FLAGS_H
> +
> +/* Bit values used to identify processor capabilities. */
> +#define FL_CO_PROC (1 << 0) /* Has external co-processor bus */
> +#define FL_ARCH3M (1 << 1) /* Extended multiply */
> +#define FL_MODE26 (1 << 2) /* 26-bit mode support */
> +#define FL_MODE32 (1 << 3) /* 32-bit mode support */
> +#define FL_ARCH4 (1 << 4) /* Architecture rel 4 */
> +#define FL_ARCH5 (1 << 5) /* Architecture rel 5 */
> +#define FL_THUMB (1 << 6) /* Thumb aware */
> +#define FL_LDSCHED (1 << 7) /* Load scheduling necessary */
> +#define FL_STRONG (1 << 8) /* StrongARM */
> +#define FL_ARCH5E (1 << 9) /* DSP extensions to v5 */
> +#define FL_XSCALE (1 << 10) /* XScale */
> +/* spare (1 << 11) */
> +#define FL_ARCH6 (1 << 12) /* Architecture rel 6. Adds
> + media instructions. */
> +#define FL_VFPV2 (1 << 13) /* Vector Floating Point V2. */
> +#define FL_WBUF (1 << 14) /* Schedule for write buffer ops.
> + Note: ARM6 & 7 derivatives only. */
> +#define FL_ARCH6K (1 << 15) /* Architecture rel 6 K extensions. */
> +#define FL_THUMB2 (1 << 16) /* Thumb-2. */
> +#define FL_NOTM (1 << 17) /* Instructions not present in the 'M'
> + profile. */
> +#define FL_THUMB_DIV (1 << 18) /* Hardware divide (Thumb mode). */
> +#define FL_VFPV3 (1 << 19) /* Vector Floating Point V3. */
> +#define FL_NEON (1 << 20) /* Neon instructions. */
> +#define FL_ARCH7EM (1 << 21) /* Instructions present in the ARMv7E-M
> + architecture. */
> +#define FL_ARCH7 (1 << 22) /* Architecture 7. */
> +#define FL_ARM_DIV (1 << 23) /* Hardware divide (ARM mode). */
> +#define FL_ARCH8 (1 << 24) /* Architecture 8. */
> +#define FL_CRC32 (1 << 25) /* ARMv8 CRC32 instructions. */
> +
> +#define FL_SMALLMUL (1 << 26) /* Small multiply supported. */
> +#define FL_NO_VOLATILE_CE (1 << 27) /* No volatile memory in IT block. */
> +
> +#define FL_IWMMXT (1 << 29) /* XScale v2 or "Intel Wireless MMX technology". */
> +#define FL_IWMMXT2 (1 << 30) /* "Intel Wireless MMX2 technology". */
> +
> +/* Flags that only effect tuning, not available instructions. */
> +#define FL_TUNE (FL_WBUF | FL_VFPV2 | FL_STRONG | FL_LDSCHED \
> + | FL_CO_PROC)
> +
> +#define FL_FOR_ARCH2 FL_NOTM
> +#define FL_FOR_ARCH3 (FL_FOR_ARCH2 | FL_MODE32)
> +#define FL_FOR_ARCH3M (FL_FOR_ARCH3 | FL_ARCH3M)
> +#define FL_FOR_ARCH4 (FL_FOR_ARCH3M | FL_ARCH4)
> +#define FL_FOR_ARCH4T (FL_FOR_ARCH4 | FL_THUMB)
> +#define FL_FOR_ARCH5 (FL_FOR_ARCH4 | FL_ARCH5)
> +#define FL_FOR_ARCH5T (FL_FOR_ARCH5 | FL_THUMB)
> +#define FL_FOR_ARCH5E (FL_FOR_ARCH5 | FL_ARCH5E)
> +#define FL_FOR_ARCH5TE (FL_FOR_ARCH5E | FL_THUMB)
> +#define FL_FOR_ARCH5TEJ FL_FOR_ARCH5TE
> +#define FL_FOR_ARCH6 (FL_FOR_ARCH5TE | FL_ARCH6)
> +#define FL_FOR_ARCH6J FL_FOR_ARCH6
> +#define FL_FOR_ARCH6K (FL_FOR_ARCH6 | FL_ARCH6K)
> +#define FL_FOR_ARCH6Z FL_FOR_ARCH6
> +#define FL_FOR_ARCH6ZK FL_FOR_ARCH6K
> +#define FL_FOR_ARCH6T2 (FL_FOR_ARCH6 | FL_THUMB2)
> +#define FL_FOR_ARCH6M (FL_FOR_ARCH6 & ~FL_NOTM)
> +#define FL_FOR_ARCH7 ((FL_FOR_ARCH6T2 & ~FL_NOTM) | FL_ARCH7)
> +#define FL_FOR_ARCH7A (FL_FOR_ARCH7 | FL_NOTM | FL_ARCH6K)
> +#define FL_FOR_ARCH7VE (FL_FOR_ARCH7A | FL_THUMB_DIV | FL_ARM_DIV)
> +#define FL_FOR_ARCH7R (FL_FOR_ARCH7A | FL_THUMB_DIV)
> +#define FL_FOR_ARCH7M (FL_FOR_ARCH7 | FL_THUMB_DIV)
> +#define FL_FOR_ARCH7EM (FL_FOR_ARCH7M | FL_ARCH7EM)
> +#define FL_FOR_ARCH8A (FL_FOR_ARCH7VE | FL_ARCH8)
> +
> +#endif /* GCC_ARM_FLAGS_H */
> diff --git a/gcc/config/arm/arm-opts.h b/gcc/config/arm/arm-opts.h
> index 039e333..dd727cf 100644
> --- a/gcc/config/arm/arm-opts.h
> +++ b/gcc/config/arm/arm-opts.h
> @@ -25,6 +25,8 @@
> #ifndef ARM_OPTS_H
> #define ARM_OPTS_H
>
> +#include "arm-flags.h"
> +
> /* The various ARM cores. */
> enum processor_type
> {
> @@ -77,4 +79,25 @@ enum arm_tls_type {
> TLS_GNU,
> TLS_GNU2
> };
> +
> +struct arm_arch_core_flag
> +{
> + const char *const name;
> + const unsigned long flags;
> +};
> +
> +static const struct arm_arch_core_flag arm_arch_core_flags[] =
> +{
> +#undef ARM_CORE
> +#define ARM_CORE(NAME, X, IDENT, ARCH, FLAGS, COSTS) \
> + {NAME, FLAGS | FL_FOR_ARCH##ARCH},
> +#include "arm-cores.def"
> +#undef ARM_CORE
> +#undef ARM_ARCH
> +#define ARM_ARCH(NAME, CORE, ARCH, FLAGS) \
> + {NAME, FLAGS},
> +#include "arm-arches.def"
> +#undef ARM_ARCH
> + {NULL, 0}
> +};
Did you consider implications from mixing ARCHes and CPUs in the same array? It should not be a problem, but would you please double-check that cases like "-march=cortex-a15" are properly caught as errors elsewhere in the driver?
> #endif
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index 28ffe52..325a81c 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -325,75 +325,6 @@ extern const char *arm_rewrite_selected_cpu (const char *name);
>
> extern bool arm_is_constant_pool_ref (rtx);
>
> -/* Flags used to identify the presence of processor capabilities. */
You've lost this comment in the new file. Was it intentional?
> -
> -/* Bit values used to identify processor capabilities. */
> -#define FL_CO_PROC (1 << 0) /* Has external co-processor bus */
> -#define FL_ARCH3M (1 << 1) /* Extended multiply */
> -#define FL_MODE26 (1 << 2) /* 26-bit mode support */
> -#define FL_MODE32 (1 << 3) /* 32-bit mode support */
> -#define FL_ARCH4 (1 << 4) /* Architecture rel 4 */
> -#define FL_ARCH5 (1 << 5) /* Architecture rel 5 */
> -#define FL_THUMB (1 << 6) /* Thumb aware */
> -#define FL_LDSCHED (1 << 7) /* Load scheduling necessary */
> -#define FL_STRONG (1 << 8) /* StrongARM */
> -#define FL_ARCH5E (1 << 9) /* DSP extensions to v5 */
> -#define FL_XSCALE (1 << 10) /* XScale */
> -/* spare (1 << 11) */
> -#define FL_ARCH6 (1 << 12) /* Architecture rel 6. Adds
> - media instructions. */
> -#define FL_VFPV2 (1 << 13) /* Vector Floating Point V2. */
> -#define FL_WBUF (1 << 14) /* Schedule for write buffer ops.
> - Note: ARM6 & 7 derivatives only. */
> -#define FL_ARCH6K (1 << 15) /* Architecture rel 6 K extensions. */
> -#define FL_THUMB2 (1 << 16) /* Thumb-2. */
> -#define FL_NOTM (1 << 17) /* Instructions not present in the 'M'
> - profile. */
> -#define FL_THUMB_DIV (1 << 18) /* Hardware divide (Thumb mode). */
> -#define FL_VFPV3 (1 << 19) /* Vector Floating Point V3. */
> -#define FL_NEON (1 << 20) /* Neon instructions. */
> -#define FL_ARCH7EM (1 << 21) /* Instructions present in the ARMv7E-M
> - architecture. */
> -#define FL_ARCH7 (1 << 22) /* Architecture 7. */
> -#define FL_ARM_DIV (1 << 23) /* Hardware divide (ARM mode). */
> -#define FL_ARCH8 (1 << 24) /* Architecture 8. */
> -#define FL_CRC32 (1 << 25) /* ARMv8 CRC32 instructions. */
> -
> -#define FL_SMALLMUL (1 << 26) /* Small multiply supported. */
> -#define FL_NO_VOLATILE_CE (1 << 27) /* No volatile memory in IT block. */
> -
> -#define FL_IWMMXT (1 << 29) /* XScale v2 or "Intel Wireless MMX technology". */
> -#define FL_IWMMXT2 (1 << 30) /* "Intel Wireless MMX2 technology". */
> -
> -/* Flags that only effect tuning, not available instructions. */
> -#define FL_TUNE (FL_WBUF | FL_VFPV2 | FL_STRONG | FL_LDSCHED \
> - | FL_CO_PROC)
> -
> -#define FL_FOR_ARCH2 FL_NOTM
> -#define FL_FOR_ARCH3 (FL_FOR_ARCH2 | FL_MODE32)
> -#define FL_FOR_ARCH3M (FL_FOR_ARCH3 | FL_ARCH3M)
> -#define FL_FOR_ARCH4 (FL_FOR_ARCH3M | FL_ARCH4)
> -#define FL_FOR_ARCH4T (FL_FOR_ARCH4 | FL_THUMB)
> -#define FL_FOR_ARCH5 (FL_FOR_ARCH4 | FL_ARCH5)
> -#define FL_FOR_ARCH5T (FL_FOR_ARCH5 | FL_THUMB)
> -#define FL_FOR_ARCH5E (FL_FOR_ARCH5 | FL_ARCH5E)
> -#define FL_FOR_ARCH5TE (FL_FOR_ARCH5E | FL_THUMB)
> -#define FL_FOR_ARCH5TEJ FL_FOR_ARCH5TE
> -#define FL_FOR_ARCH6 (FL_FOR_ARCH5TE | FL_ARCH6)
> -#define FL_FOR_ARCH6J FL_FOR_ARCH6
> -#define FL_FOR_ARCH6K (FL_FOR_ARCH6 | FL_ARCH6K)
> -#define FL_FOR_ARCH6Z FL_FOR_ARCH6
> -#define FL_FOR_ARCH6ZK FL_FOR_ARCH6K
> -#define FL_FOR_ARCH6T2 (FL_FOR_ARCH6 | FL_THUMB2)
> -#define FL_FOR_ARCH6M (FL_FOR_ARCH6 & ~FL_NOTM)
> -#define FL_FOR_ARCH7 ((FL_FOR_ARCH6T2 & ~FL_NOTM) | FL_ARCH7)
> -#define FL_FOR_ARCH7A (FL_FOR_ARCH7 | FL_NOTM | FL_ARCH6K)
> -#define FL_FOR_ARCH7VE (FL_FOR_ARCH7A | FL_THUMB_DIV | FL_ARM_DIV)
> -#define FL_FOR_ARCH7R (FL_FOR_ARCH7A | FL_THUMB_DIV)
> -#define FL_FOR_ARCH7M (FL_FOR_ARCH7 | FL_THUMB_DIV)
> -#define FL_FOR_ARCH7EM (FL_FOR_ARCH7M | FL_ARCH7EM)
> -#define FL_FOR_ARCH8A (FL_FOR_ARCH7VE | FL_ARCH8)
> -
Would you please explicitly include "arm-flags.h" into arm-protos.h? There is an effort underway to flatten header files (make sure that no header includes another header), so it is best to minimize implicit includes.
> /* The bits in this mask specify which
> instructions we are allowed to generate. */
> extern unsigned long insn_flags;
> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
> index 8c10ea3..3119957 100644
> --- a/gcc/config/arm/arm.h
> +++ b/gcc/config/arm/arm.h
> @@ -2394,13 +2394,18 @@ extern const char *arm_rewrite_mcpu (int argc, const char **argv);
> " :%{march=*:-march=%*}}" \
> BIG_LITTLE_SPEC
>
> +extern const char *arm_is_target_thumb_only (int argc, const char **argv);
> +#define MODE_SET_SPEC_FUNCTIONS \
> + { "target_mode_check", arm_is_target_thumb_only },
Consider renaming to TARGET_MODE_SPEC_FUNCTIONS or THUMBONLY_SPEC_FUNCTIONS. Current name sounds like "set spec functions for a given mode", while what it does is "these are spec functions for setting mode".
> +
> /* -mcpu=native handling only makes sense with compiler running on
> an ARM chip. */
> #if defined(__arm__)
> extern const char *host_detect_local_cpu (int argc, const char **argv);
> # define EXTRA_SPEC_FUNCTIONS \
> { "local_cpu_detect", host_detect_local_cpu }, \
> - BIG_LITTLE_CPU_SPEC_FUNCTIONS
> + BIG_LITTLE_CPU_SPEC_FUNCTIONS \
> + MODE_SET_SPEC_FUNCTIONS
>
> # define MCPU_MTUNE_NATIVE_SPECS \
> " %{march=native:%<march=native %:local_cpu_detect(arch)}" \
> @@ -2408,9 +2413,19 @@ extern const char *host_detect_local_cpu (int argc, const char **argv);
> " %{mtune=native:%<mtune=native %:local_cpu_detect(tune)}"
> #else
> # define MCPU_MTUNE_NATIVE_SPECS ""
> -# define EXTRA_SPEC_FUNCTIONS BIG_LITTLE_CPU_SPEC_FUNCTIONS
> +# define EXTRA_SPEC_FUNCTIONS \
> + BIG_LITTLE_CPU_SPEC_FUNCTIONS \
> + MODE_SET_SPEC_FUNCTIONS
> #endif
>
> -#define DRIVER_SELF_SPECS MCPU_MTUNE_NATIVE_SPECS
> +/* Automatically add -mthumb for thumb-only target if mode isn't specified
> + via configuration option --with-mode and command line.
> + If -march present, we collect just -march options. Otherwise we
> + collect just -mcpu options. The last one of them will be used to
> + decide target mode. */
> +#define MODE_SET_SPECS \
> + " %{!marm:%{!mthumb:%:target_mode_check(%{march=*:%*;mcpu=*:%*})}}"
Again, consider renaming to "TARGET_MODE_SPECS" or THUMBONLY_SPEC_FUNCTIONS.
> +
> +#define DRIVER_SELF_SPECS MCPU_MTUNE_NATIVE_SPECS MODE_SET_SPECS
> #define TARGET_SUPPORTS_WIDE_INT 1
> #endif /* ! GCC_ARM_H */
>
Thanks,
--
Maxim Kuvyrkov
www.linaro.org
More information about the Gcc-patches
mailing list