diff mbox

[ARC] New option handling, refurbish multilib support.

Message ID 20161103225849.GG31714@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Nov. 3, 2016, 10:58 p.m. UTC 
* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [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

Comments

Claudiu Zissulescu Nov. 4, 2016, 11:40 a.m. UTC | #1 
Andrew,

> 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.

Thanks for the suggestion.

> 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?

I need a value to mark if the variable in question is changed by a command line option or not. This is required when we set the cpu's specific configuration. I'll include this comment in the description of the define for clarity.
 
> 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".

This patch was crafted way before the new ARC binutils to be upstreamed. Hence, it needed to work with the "old" binutils which had no support for 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.

I am preparing a patch to binutils which should be able to recognize all GCC's cpu variations as valid .cpu pseudo-op argument. Hence, we can just pass to the assembler the appropriate .cpu variation seamlessly. If it is ok with you, I would like to come with a new patch on this topic after I extend .cpu support in binutils.  Alternatively, I can include your suggestion now, but then I will refurbish it later on. Please let me know what do u fancy most.

Thanks,
Claudiu
diff mbox

Patch

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";