From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5338 invoked by alias); 4 Mar 2015 08:16:44 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 5325 invoked by uid 89); 4 Mar 2015 08:16:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-la0-f49.google.com Received: from mail-la0-f49.google.com (HELO mail-la0-f49.google.com) (209.85.215.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 04 Mar 2015 08:16:41 +0000 Received: by lams18 with SMTP id s18so42843542lam.13 for ; Wed, 04 Mar 2015 00:16:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:content-type:mime-version:subject:from :in-reply-to:date:cc:content-transfer-encoding:message-id:references :to; bh=l0TiUz3kXtnSczdodT31sRi0ABO/82xW2RTv2CL6q3c=; b=KQfxon+ThicuaephqwJl7S9uvxeOZdY5WoOagt6Wbxxcxnw5RLz/yWcNRnWGQFuvLs n3l1bh85qjC/KBaw9CNa/iQ6AFN0YBXUThllfzhX2bEGYH9YJN4Tj99KoP7U++AVA9ou sKQtJvm95jXZ5biY7xoieqd5+0cHf4XcDZiGe12WbsaERUo2PjNOj4Rtk1Jjdb4026i4 yuI+rzUYutstYOrYo/hwVxzhBaMCv4ZwyiiWEMn/fbZeAROlTtvDvslzQqCfw8En+ISi Ff1a9b1MFGL4vr1Y9knmljl2tKmLSzBbe2dWw1rR8HDgyq9wI01m8yNJ/yLKD0X1Tuv1 96MQ== X-Gm-Message-State: ALoCoQn6IxDXFiWOmrIKQk0yeKzAiGO88VkkGgrV+P0NVptJaDxaWDPj9BQauJ7fpse+4grsLi22 X-Received: by 10.112.204.197 with SMTP id la5mr2265251lbc.29.1425456997826; Wed, 04 Mar 2015 00:16:37 -0800 (PST) Received: from [192.168.1.118] ([90.154.65.7]) by mx.google.com with ESMTPSA id h7sm624886lbj.29.2015.03.04.00.16.36 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 04 Mar 2015 00:16:37 -0800 (PST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2070.6\)) Subject: Re: [PATCH][ARM]Automatically add -mthumb for thumb-only target when mode isn't specified From: Maxim Kuvyrkov In-Reply-To: Date: Wed, 04 Mar 2015 08:16:00 -0000 Cc: GCC Patches , Ramana Radhakrishnan , Richard Earnshaw Content-Transfer-Encoding: quoted-printable Message-Id: <4FE4F7E6-5093-4E37-A2D6-93272E679C69@linaro.org> References: <000001d0548a$789a1bd0$69ce5370$@arm.com> <0E847550-2947-46C3-991C-2720641BC881@linaro.org> To: Terry Guo X-SW-Source: 2015-03/txt/msg00187.txt.bz2 > On Mar 4, 2015, at 5:46 AM, Terry Guo wrote: >=20 > On Wed, Mar 4, 2015 at 10:44 AM, Terry Guo wrote: >> On Mon, Mar 2, 2015 at 9:08 PM, Maxim Kuvyrkov >> wrote: >>>> On Mar 2, 2015, at 4:44 AM, Terry Guo wrote: >>>>=20 >>>> Hi there, >>>>=20 >>>> 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? >>>>=20 >>>> BR, >>>> Terry >>>>=20 >>>> gcc/ChangeLog: >>>>=20 >>>> 2015-03-02 Terry Guo >>>>=20 >>>> * 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. >>>=20 >>> Did you consider approach of implementing this purely inside cc1 rather= than driver? >>>=20 >>> We do not seem to need to pass -mthumb to assembler or linker since tho= se will pick up ARM-ness / Thumb-ness from function annotations. Therefore= we need to handle -marm / -mthumb for cc1 only. What am I missing? >>>=20 >>> Also, what's the significance of moving FL_* flags to arm-opts.h? If y= ou had to separate FL_* definitions from the rest of arm-protos.h, then a n= ew dedicated file (e.g., arm-fl.h) would be a better choice for new home of= FL_* definitions. >>>=20 >>=20 >> 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 t= he 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 re= view, 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/a= rm-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]); > } >=20=20 > +/* 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 =3D 0; opt < (ARRAY_SIZE (arm_arch_core_flags) - 1); opt+= +) > + if ((strcmp (argv[argc - 1], arm_arch_core_flags[opt].name) =3D=3D 0) > + && ((arm_arch_core_flags[opt].flags & FL_NOTM) =3D=3D 0)) > + return "-mthumb"; > + > + return NULL; > + } > + else > + return NULL; > +} > + > #undef ARM_CPU_NAME_LENGTH >=20=20 >=20=20 > 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.=20 > + > + 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 > + . */ > + > +#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 ARM= v7E-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 MM= X 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 >=20=20 > +#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[] =3D > +{ > +#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=3Dcortex-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 c= har *name); >=20=20 > extern bool arm_is_constant_pool_ref (rtx); >=20=20 > -/* 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 ARM= v7E-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 MM= X 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 in= cludes 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, co= nst char **argv); > " :%{march=3D*:-march=3D%*}}" \ > BIG_LITTLE_SPEC >=20=20 > +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 wh= at it does is "these are spec functions for setting mode". > + > /* -mcpu=3Dnative 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 >=20=20 > # define MCPU_MTUNE_NATIVE_SPECS \ > " %{march=3Dnative:% @@ -2408,9 +2413,19 @@ extern const char *host_detect_local_cpu (int argc= , const char **argv); > " %{mtune=3Dnative:% #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 >=20=20 > -#define DRIVER_SELF_SPECS MCPU_MTUNE_NATIVE_SPECS > +/* Automatically add -mthumb for thumb-only target if mode isn't specifi= ed > + 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=3D*:%*;mcpu=3D*:%*})}}" 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 */ >=20 Thanks, -- Maxim Kuvyrkov www.linaro.org