From patchwork Thu Nov 3 22:58:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 691067 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3t90lb4VLWz9vDS for ; Fri, 4 Nov 2016 09:59:21 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=E8htMynD; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=aZ3VJZ5K4ut+pINRk Euc4IDkIun4/aDZQnkmYVkV1FzghxsgOI3iEblH1UAZvfMHiDFEzxZ3cJU0quIm1 vyDvK1gwpuKgPYpI/nxFoAtk0/Z56mYmS5dVaULirSGEDakYF4ZKB55CkQt/TaPa 4UjmHkw6t0KJEfO+dYHU6+B5II= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=l36b+/DCQy4Lv0V4BUm3A+O DDcM=; b=E8htMynDxw0B1QkdqLAXdRGfR18mSOCJAqvkDue0avfsTDHisWZYlnt 6bylSIIZQ3PsSLFjJadvPfOf8ju+rzbBB1FCuG783Dn9NgPKuRj4KWmcC5PtRak4 O3RFBPodCjvmETA492ff1kF6r2tLKZEe3Z1gU9NHdf2j8SBfrHto= Received: (qmail 129241 invoked by alias); 3 Nov 2016 22:59:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 129231 invoked by uid 89); 3 Nov 2016 22:59:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM autolearn=no version=3.3.2 spammy=8007, significance, Zissulescu, MASK X-HELO: mail-wm0-f45.google.com Received: from mail-wm0-f45.google.com (HELO mail-wm0-f45.google.com) (74.125.82.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 03 Nov 2016 22:59:02 +0000 Received: by mail-wm0-f45.google.com with SMTP id p190so16890261wmp.1 for ; Thu, 03 Nov 2016 15:59:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+fsyV8jVmXvItWtYuhJX1Qc6dQLtV25N9NX2jlboN9c=; b=ilhuLGJerc4TFhINqUDjzDB7gI4MeTciYUfO05xANQJ/FUgR+NmxCZDXTy6qIZIyWG SKVodVjIdb6k0yx7unHTm2R1OVixux2vMG29MQm9bYihgHzhOFmvAHNlicLVbq+kRrxa jo4nXHXlcc38iNbvU16yXl4wUbTQfuX1TLa2yG0tR6JnxV7d8NqIFgpEUTI6WY6rPQqm khoV9uM+xTYxyApS+tteVlPhl0xV7847Hrl/bJnAkm1G6HTWtzvxzKU1seZMCUMZQ335 t6ivmfZVQknSkCtViNVC/Mp6Bu+nV9b2t4RGrXlBcAdX073SfowhD8fL4KvXSvaM5456 OyQA== X-Gm-Message-State: ABUngvdu1i10SU01GsH8h5Qu2IZjmxpPFsEhmHV04BONWsu7Re5tww7kQSg5GmAcXYUCXQ== X-Received: by 10.28.71.1 with SMTP id u1mr289922wma.128.1478213939743; Thu, 03 Nov 2016 15:58:59 -0700 (PDT) Received: from localhost (host86-164-199-110.range86-164.btcentralplus.com. [86.164.199.110]) by smtp.gmail.com with ESMTPSA id ia7sm10749482wjb.23.2016.11.03.15.58.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 03 Nov 2016 15:58:58 -0700 (PDT) Date: Thu, 3 Nov 2016 22:58:49 +0000 From: Andrew Burgess To: Claudiu Zissulescu Cc: gcc-patches@gcc.gnu.org, Francois.Bedard@synopsys.com Subject: Re: [PATCH] [ARC] New option handling, refurbish multilib support. Message-ID: <20161103225849.GG31714@embecosm.com> References: <20161012200208.GB18222@embecosm.com> <1477928777-19606-1-git-send-email-claziss@synopsys.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1477928777-19606-1-git-send-email-claziss@synopsys.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.6.1 (2016-04-27) X-IsSubscribed: yes * Claudiu Zissulescu [2016-10-31 16:46:17 +0100]: > Please find the updated patch. > > What is new: > - The .def files are having a comment block on how to add new lines. > - The arc_seen_option is not used. > - The arc_cpu* variables are not used. > > Please let me know if I miss something, Claudiu, Thanks for the refresh on this patch, I think that it's looking really great. I just had a couple of issues that I think would be worth addressing before we merge this. In this hunk: > diff --git a/gcc/config/arc/arc.opt b/gcc/config/arc/arc.opt > index 4caf366..20a526b 100644 > --- a/gcc/config/arc/arc.opt > +++ b/gcc/config/arc/arc.opt > @@ -53,9 +53,12 @@ mARC700 > Target Report > Same as -mA7. > > +TargetVariable > +int arc_mpy_option = DEFAULT_arc_mpy_option > + > mmpy-option= > -Target RejectNegative Joined UInteger Var(arc_mpy_option) Init(2) > --mmpy-option={0,1,2,3,4,5,6,7,8,9} Compile ARCv2 code with a multiplier design option. Option 2 is default on. > +Target RejectNegative Joined > +-mmpy-option=MPY Compile ARCv2 code with a multiplier design option. > > mdiv-rem > Target Report Mask(DIVREM) > @@ -100,7 +103,7 @@ Target Report Mask(MUL64_SET) you create the TargetVariable arc_mpy_option, however, I think it would be neater to fold this variable into the mmpy-option as it was before, but, changing the type to Enum. This would allow the big option checking switch to be removed from arc-common.c, which I think is a win overall. I wasn't 100% certain that the above would actually be possible, so I put together a prototype, I've included the patch below, that applies on top of your patch. Hopefully you'll agree that it's a nice clean up. My second question was with this hunk: > diff --git a/gcc/config/arc/arc-opts.h b/gcc/config/arc/arc-opts.h > index cbd7898..81446b4 100644 > --- a/gcc/config/arc/arc-opts.h > +++ b/gcc/config/arc/arc-opts.h > @@ -48,3 +49,35 @@ enum processor_type > /* Double precision floating point assist operations. */ > #define FPX_DP 0x0100 > > +/* fpus option combi. */ > +#define FPU_FPUS (FPU_SP | FPU_SC) > +/* fpud option combi. */ > +#define FPU_FPUD (FPU_SP | FPU_SC | FPU_DP | FPU_DC) > +/* fpuda option combi. */ > +#define FPU_FPUDA (FPU_SP | FPU_SC | FPX_DP) > +/* fpuda_div option combi. */ > +#define FPU_FPUDA_DIV (FPU_SP | FPU_SC | FPU_SD | FPX_DP) > +/* fpuda_fma option combi. */ > +#define FPU_FPUDA_FMA (FPU_SP | FPU_SC | FPU_SF | FPX_DP) > +/* fpuda_all option combi. */ > +#define FPU_FPUDA_ALL (FPU_SP | FPU_SC | FPU_SF | FPU_SD | FPX_DP) > +/* fpus_div option combi. */ > +#define FPU_FPUS_DIV (FPU_SP | FPU_SC | FPU_SD) > +/* fpus_fma option combi. */ > +#define FPU_FPUS_FMA (FPU_SP | FPU_SC | FPU_SF) > +/* fpus_all option combi. */ > +#define FPU_FPUS_ALL (FPU_SP | FPU_SC | FPU_SF | FPU_SD) > +/* fpud_div option combi. */ > +#define FPU_FPUD_DIV (FPU_FPUS_DIV | FPU_DP | FPU_DC | FPU_DD) > +/* fpud_fma option combi. */ > +#define FPU_FPUD_FMA (FPU_FPUS_FMA | FPU_DP | FPU_DC | FPU_DF) > +/* fpud_all option combi. */ > +#define FPU_FPUD_ALL (FPU_FPUS_ALL | FPU_DP | FPU_DC | FPU_DF | FPU_DD) > + > +/* Default FPU option value. */ > +#define DEFAULT_arc_fpu_build 0x10000000 > + > +/* Default MPY option value. */ > +#define DEFAULT_arc_mpy_option -1 > + > +#endif /* ARC_OPTS_H */ I wonder where the vale 0x10000000 comes from, what's the significance of it. I could ask the same question about the magic -1 constant, but it's rather more obvious that -1 is just a no-value-selected magic number. I guess my questions for 0x10000000 are why this specific value? What does it mean? My final question concerns: > diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c > index 5e8d6b4..8810e91 100644 > --- a/gcc/config/arc/arc.c > +++ b/gcc/config/arc/arc.c > @@ -853,7 +782,86 @@ static void > arc_override_options (void) > { > if (arc_cpu == PROCESSOR_NONE) > - arc_cpu = PROCESSOR_ARC700; > + arc_cpu = TARGET_CPU_DEFAULT; > + > + /* Set the default cpu options. */ > + arc_selected_cpu = &arc_cpu_types[(int) arc_cpu]; > + arc_selected_arch = &arc_arch_types[(int) arc_selected_cpu->arch]; > + arc_base_cpu = arc_selected_arch->arch; > + > + /* Set the architectures. */ > + switch (arc_selected_arch->arch) > + { > + case BASE_ARCH_em: > + arc_cpu_string = "EM"; > + break; > + case BASE_ARCH_hs: > + arc_cpu_string = "HS"; > + break; > + case BASE_ARCH_700: > + arc_cpu_string = "ARC700"; > + break; > + case BASE_ARCH_6xx: > + arc_cpu_string = "ARC600"; > + break; > + default: > + gcc_unreachable (); > + } > + > + /* Set cpu flags accordingly to architecture/selected cpu. The cpu > + specific flags are set in arc-common.c. The architecture forces > + the default hardware configurations in, regardless what command > + line options are saying. The CPU optional hw options can be > + turned on or off. */ > +#define ARC_OPT(NAME, CODE, MASK, DOC) \ > + do { \ > + if ((arc_selected_cpu->flags & CODE) \ > + && ((target_flags_explicit & MASK) == 0)) \ > + target_flags |= MASK; \ > + if (arc_selected_arch->dflags & CODE) \ > + target_flags |= MASK; \ > + } while (0); > +#define ARC_OPTX(NAME, CODE, VAR, VAL, DOC) \ > + do { \ > + if ((arc_selected_cpu->flags & CODE) \ > + && (VAR == DEFAULT_##VAR)) \ > + VAR = VAL; \ > + if (arc_selected_arch->dflags & CODE) \ > + VAR = VAL; \ > + } while (0); > + > +#include "arc-options.def" > + > +#undef ARC_OPTX > +#undef ARC_OPT > + > + /* Check options against architecture options. Throw an error if > + option is not allowed. */ > +#define ARC_OPTX(NAME, CODE, VAR, VAL, DOC) \ > + do { \ > + if ((VAR == VAL) \ > + && (!(arc_selected_arch->flags & CODE))) \ > + { \ > + error ("%s is not available for %s architecture", \ > + DOC, arc_selected_arch->name); \ > + } \ > + } while (0); > +#define ARC_OPT(NAME, CODE, MASK, DOC) \ > + do { \ > + if ((target_flags & MASK) \ > + && (!(arc_selected_arch->flags & CODE))) \ > + error ("%s is not available for %s architecture", \ > + DOC, arc_selected_arch->name); \ > + } while (0); > + > +#include "arc-options.def" > + > +#undef ARC_OPTX > +#undef ARC_OPT > + > + /* Set Tune option. */ > + if (arc_tune == TUNE_NONE) > + arc_tune = (enum attr_tune) arc_selected_cpu->tune; > > if (arc_size_opt_level == 3) > optimize_size = 1; and also the driver-arc.c file. You seem to be missing support for nps400 in here. Specifically, if I pass -mcpu=nps400 to GCC I'd expect the generated assembler file to include ".cpu NPS400", and the assembler to be driven with "-mcpu=nps400". Admittedly we're missing a GCC test for this (there's a patch below for just such a new test). I think the solution could be fairly easy, if we tracked the specific processor type in arc_cpu_t structure we could specialise in arc_override_options and in driver-arc.c, though I don't know if you'd agree that this is the right approach... I'm not entirely sure myself... but it might be the easiest approach to move us forward. Anyway, I include a patch for that below too, feel free to use or not as you see fit. Everything else looks fine. Thanks, Andrew --- Patch #1: Changes to option handling diff --git a/gcc/common/config/arc/arc-common.c b/gcc/common/config/arc/arc-common.c index bc97411..07f0a65 100644 --- a/gcc/common/config/arc/arc-common.c +++ b/gcc/common/config/arc/arc-common.c @@ -71,7 +71,6 @@ arc_handle_option (struct gcc_options *opts, int value = decoded->value; const char *arg = decoded->arg; static int mcpu_seen = PROCESSOR_NONE; - char *p; switch (code) { @@ -85,45 +84,8 @@ arc_handle_option (struct gcc_options *opts, break; case OPT_mmpy_option_: - p = ASTRDUP (arg); - - if (!strcmp (p, "0") - || !strcmp (p, "none")) - opts->x_arc_mpy_option = 0; - else if (!strcmp (p, "1") - || !strcmp (p, "w")) - { - opts->x_arc_mpy_option = 1; - warning_at (loc, 0, "Unsupported value for mmpy-option"); - } - else if (!strcmp (p, "2") - || !strcmp (p, "mpy") - || !strcmp (p, "wlh1")) - opts->x_arc_mpy_option = 2; - else if (!strcmp (p, "3") - || !strcmp (p, "wlh2")) - opts->x_arc_mpy_option = 3; - else if (!strcmp (p, "4") - || !strcmp (p, "wlh3")) - opts->x_arc_mpy_option = 4; - else if (!strcmp (p, "5") - || !strcmp (p, "wlh4")) - opts->x_arc_mpy_option = 5; - else if (!strcmp (p, "6") - || !strcmp (p, "wlh5")) - opts->x_arc_mpy_option = 6; - else if (!strcmp (p, "7") - || !strcmp (p, "plus_dmpy")) - opts->x_arc_mpy_option = 7; - else if (!strcmp (p, "8") - || !strcmp (p, "plus_macd")) - opts->x_arc_mpy_option = 8; - else if (!strcmp (p, "9") - || !strcmp (p, "plus_qmacw")) - opts->x_arc_mpy_option = 9; - else - error_at (loc, "unknown value %qs for -mmpy-option", arg); - + if (opts->x_arc_mpy_option == 1) + warning_at (loc, 0, "Unsupported value for mmpy-option"); break; default: diff --git a/gcc/config/arc/arc.opt b/gcc/config/arc/arc.opt index 20a526b..5685100 100644 --- a/gcc/config/arc/arc.opt +++ b/gcc/config/arc/arc.opt @@ -53,13 +53,76 @@ mARC700 Target Report Same as -mA7. -TargetVariable -int arc_mpy_option = DEFAULT_arc_mpy_option - mmpy-option= -Target RejectNegative Joined +Target RejectNegative Joined Enum(arc_mpy) Var(arc_mpy_option) Init(DEFAULT_arc_mpy_option) -mmpy-option=MPY Compile ARCv2 code with a multiplier design option. +Enum +Name(arc_mpy) Type(int) + +EnumValue +Enum(arc_mpy) String(0) Value(0) + +EnumValue +Enum(arc_mpy) String(none) Value(0) Canonical + +EnumValue +Enum(arc_mpy) String(1) Value(1) + +EnumValue +Enum(arc_mpy) String(w) Value(1) Canonical + +EnumValue +Enum(arc_mpy) String(2) Value(2) + +EnumValue +Enum(arc_mpy) String(mpy) Value(2) + +EnumValue +Enum(arc_mpy) String(wlh1) Value(2) Canonical + +EnumValue +Enum(arc_mpy) String(3) Value(3) + +EnumValue +Enum(arc_mpy) String(wlh2) Value(3) Canonical + +EnumValue +Enum(arc_mpy) String(4) Value(4) + +EnumValue +Enum(arc_mpy) String(wlh3) Value(4) Canonical + +EnumValue +Enum(arc_mpy) String(5) Value(5) + +EnumValue +Enum(arc_mpy) String(wlh4) Value(5) Canonical + +EnumValue +Enum(arc_mpy) String(6) Value(6) + +EnumValue +Enum(arc_mpy) String(wlh5) Value(6) Canonical + +EnumValue +Enum(arc_mpy) String(7) Value(7) + +EnumValue +Enum(arc_mpy) String(plus_dmpy) Value(7) Canonical + +EnumValue +Enum(arc_mpy) String(8) Value(8) + +EnumValue +Enum(arc_mpy) String(plus_macd) Value(8) Canonical + +EnumValue +Enum(arc_mpy) String(9) Value(9) + +EnumValue +Enum(arc_mpy) String(plus_qmacw) Value(9) Canonical + mdiv-rem Target Report Mask(DIVREM) Enable DIV-REM instructions for ARCv2. -- 2.6.4 --- Patch #2: New test for nps400 .cpu flag diff --git a/gcc/testsuite/gcc.target/arc/nps400-cpu-flag.c b/gcc/testsuite/gcc.target/arc/nps400-cpu-flag.c new file mode 100644 index 0000000..fe80ce5 --- /dev/null +++ b/gcc/testsuite/gcc.target/arc/nps400-cpu-flag.c @@ -0,0 +1,4 @@ +/* { dg-do compile } */ +/* { dg-options "-mcpu=nps400" } */ + +/* { dg-final { scan-assembler ".cpu NPS400" } } */ -- 2.6.4 --- Patch #3: Track specific processor type diff --git a/gcc/config/arc/arc-arch.h b/gcc/config/arc/arc-arch.h index 7994543..bfd3f23 100644 --- a/gcc/config/arc/arc-arch.h +++ b/gcc/config/arc/arc-arch.h @@ -68,6 +68,9 @@ typedef struct /* Architecture class. */ enum base_architecture arch; + /* Specific processor type. */ + enum processor_type processor; + /* Specific flags. */ const unsigned long long flags; @@ -108,12 +111,12 @@ const arc_arch_t arc_arch_types[] = const arc_cpu_t arc_cpu_types[] = { - {"none", BASE_ARCH_NONE, 0, ARC_TUNE_NONE}, + {"none", BASE_ARCH_NONE, PROCESSOR_NONE, 0, ARC_TUNE_NONE}, #define ARC_CPU(NAME, ARCH, FLAGS, TUNE) \ - {#NAME, BASE_ARCH_##ARCH, FLAGS, ARC_TUNE_##TUNE}, + {#NAME, BASE_ARCH_##ARCH, PROCESSOR_##NAME, FLAGS, ARC_TUNE_##TUNE}, #include "arc-cpus.def" #undef ARC_CPU - {NULL, BASE_ARCH_END, 0, ARC_TUNE_NONE} + {NULL, BASE_ARCH_END, PROCESSOR_NONE, 0, ARC_TUNE_NONE} }; #endif diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c index c075bcb..3bce7ef 100644 --- a/gcc/config/arc/arc.c +++ b/gcc/config/arc/arc.c @@ -800,7 +800,10 @@ arc_override_options (void) arc_cpu_string = "HS"; break; case BASE_ARCH_700: - arc_cpu_string = "ARC700"; + if (arc_selected_cpu->processor == PROCESSOR_nps400) + arc_cpu_string = "NPS400"; + else + arc_cpu_string = "ARC700"; break; case BASE_ARCH_6xx: arc_cpu_string = "ARC600"; diff --git a/gcc/config/arc/driver-arc.c b/gcc/config/arc/driver-arc.c index 6117968..0c24cda 100644 --- a/gcc/config/arc/driver-arc.c +++ b/gcc/config/arc/driver-arc.c @@ -64,7 +64,10 @@ arc_cpu_to_as (int argc, const char **argv) case BASE_ARCH_hs: return "-mcpu=archs"; case BASE_ARCH_700: - return "-mcpu=arc700 -mEA"; + if (arc_selected_cpu->processor == PROCESSOR_nps400) + return "-mcpu=nps400 -mEA"; + else + return "-mcpu=arc700 -mEA"; case BASE_ARCH_6xx: if (arc_selected_cpu->flags & FL_MUL64) return "-mcpu=arc600 -mmul64 -mnorm";