* Claudiu Zissulescu <[email protected]> [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";
--
2.6.4
