RE: [PATCH] [ARC] New option handling, refurbish multilib support.

[Claudiu Zissulescu]

> This looks fine.  Thanks for all your effort revising this patch.
> 
> Andrew
> 

Committed r242425. 

Thank you for your review,
Claudiu

RE: [PATCH] [ARC] New option handling, refurbish multilib support.

[Claudiu Zissulescu]

> HEAD binutils supports '.cpu NPS400' and HEAD GCC also correctly emits
> '.cpu NPS400' (when -mcpu=nps400 is passed).  After your change we no
> longer correctly emit '.cpu NPS400'.
> 

Sorry for this miss-understanding. Updating the patch to emit .cpu NSPS400.

Thanks,
Claudiu

Re: [PATCH] [ARC] New option handling, refurbish multilib support.

[Andrew Burgess]

* Claudiu Zissulescu  [2016-11-10 12:02:34 
+0100]:

> Hi,
> 
> Please find the revised patch which includes the refurbishing of
> mmpy-option option, and a new comment on DEFAULT_arc_fpu_build
> define. As for the last suggestion, my proposal is to have a latter
> patch on the topic of .cpu, synced with a related binutils patch.

I don't understand this last point.

HEAD binutils supports '.cpu NPS400' and HEAD GCC also correctly emits
'.cpu NPS400' (when -mcpu=nps400 is passed).  After your change we no
longer correctly emit '.cpu NPS400'.

I don't understand what syncing is required with binutils?

Thanks,
Andrew




> 
> OK to apply?
> Claudiu
> 
> gcc/
> 2016-05-09  Claudiu Zissulescu  
> 
>   * config/arc/arc-arch.h: New file.
>   * config/arc/arc-arches.def: Likewise.
>   * config/arc/arc-cpus.def: Likewise.
>   * config/arc/arc-options.def: Likewise.
>   * config/arc/t-multilib: Likewise.
>   * config/arc/genmultilib.awk: Likewise.
>   * config/arc/genoptions.awk: Likewise.
>   * config/arc/arc-tables.opt: Likewise.
>   * config/arc/driver-arc.c: Likewise.
>   * common/config/arc/arc-common.c (arc_handle_option): Trace
>   toggled options.
>   * config.gcc (arc*-*-*): Add arc-tables.opt to arc's extra
>   options; check for supported cpu against arc-cpus.def file.
>   (arc*-*-elf*, arc*-*-linux-uclibc*): Use new make fragment; define
>   TARGET_CPU_BUILD macro; add driver-arc.o as an extra object.
>   * config/arc/arc-c.def: Add emacs local variables.
>   * config/arc/arc-opts.h (processor_type): Use arc-cpus.def file.
>   (FPU_FPUS, FPU_FPUD, FPU_FPUDA, FPU_FPUDA_DIV, FPU_FPUDA_FMA)
>   (FPU_FPUDA_ALL, FPU_FPUS_DIV, FPU_FPUS_FMA, FPU_FPUS_ALL)
>   (FPU_FPUD_DIV, FPU_FPUD_FMA, FPU_FPUD_ALL): New defines.
>   (DEFAULT_arc_fpu_build): Define.
>   (DEFAULT_arc_mpy_option): Define.
>   * config/arc/arc-protos.h (arc_init): Delete.
>   * config/arc/arc.c (arc_cpu_name): New variable.
>   (arc_selected_cpu, arc_selected_arch, arc_arcem, arc_archs)
>   (arc_arc700, arc_arc600, arc_arc601): New variable.
>   (arc_init): Add static; remove selection of default tune value,
>   cleanup obsolete error messages.
>   (arc_override_options): Make use of .def files for selecting the
>   right cpu and option configurations.
>   * config/arc/arc.h (stdbool.h): Include.
>   (TARGET_CPU_DEFAULT): Define.
>   (CPP_SPEC): Remove mcpu=NPS400 handling.
>   (arc_cpu_to_as): Declare.
>   (EXTRA_SPEC_FUNCTIONS): Define.
>   (OPTION_DEFAULT_SPECS): Likewise.
>   (ASM_DEFAULT): Remove.
>   (ASM_SPEC): Use arc_cpu_to_as.
>   (DRIVER_SELF_SPECS): Remove deprecated options.
>   (arc_base_cpu): Declare.
>   (TARGET_ARC600, TARGET_ARC601, TARGET_ARC700, TARGET_EM)
>   (TARGET_HS, TARGET_V2, TARGET_ARC600): Make them use arc_base_cpu
>   variable.
>   (MULTILIB_DEFAULTS): Use ARC_MULTILIB_CPU_DEFAULT.
>   * config/arc/arc.md (attr_cpu): Remove.
>   * config/arc/arc.opt (mno-mpy): Deprecate.
>   (mcpu=ARC600, mcpu=ARC601, mcpu=ARC700, mcpu=NPS400, mcpu=ARCEM)
>   (mcpu=ARCHS): Remove.
>   (mcrc, mdsp-packa, mdvbf, mmac-d16, mmac-24, mtelephony, mrtsc):
>   Deprecate.
>   (mbarrel_shifte, mspfp_, mdpfp_, mdsp_pack, mmac_): Remove.
>   (arc_fpu): Use new defines.
>   (mpy-option): Change to use numeric or string like inputs.
>   * config/arc/t-arc (driver-arc.o): New target.
>   (arc-cpus, t-multilib, arc-tables.opt): Likewise.
>   * config/arc/t-arc-newlib: Delete.
>   * config/arc/t-arc-uClibc: Renamed to t-uClibc.
>   * doc/invoke.texi (ARC): Update arc options.
> ---
>  gcc/common/config/arc/arc-common.c |  69 -
>  gcc/config.gcc |  47 +
>  gcc/config/arc/arc-arch.h  | 120 ++
>  gcc/config/arc/arc-arches.def  |  56 ++
>  gcc/config/arc/arc-c.def   |   4 +
>  gcc/config/arc/arc-cpus.def|  75 ++
>  gcc/config/arc/arc-options.def | 109 
>  gcc/config/arc/arc-opts.h  |  49 +++--
>  gcc/config/arc/arc-protos.h|   1 -
>  gcc/config/arc/arc-tables.opt  |  90 
>  gcc/config/arc/arc.c   | 176 +---
>  gcc/config/arc/arc.h   |  89 
>  gcc/config/arc/arc.md  |   5 -
>  gcc/config/arc/arc.opt | 169 +++---
>  gcc/config/arc/driver-arc.c|  78 ++
>  gcc/config/arc/genmultilib.awk | 203 
> +
>  gcc/config/arc/genoptions.awk  |  86 
>  gcc/config/arc/t-arc   |  19 
>  gcc/config/arc/t-arc-newlib|  46 -
>  gcc/config/arc/t-arc-uClibc|  20 
>  

RE: [PATCH] [ARC] New option handling, refurbish multilib support.

[Claudiu Zissulescu]

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 0x1000 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 0x1000 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

Re: [PATCH] [ARC] New option handling, refurbish multilib support.

[Andrew Burgess]

* 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_DP0x0100
>  
> +/* 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 0x1000
> +
> +/* Default MPY option value.  */
> +#define DEFAULT_arc_mpy_option -1
> +
> +#endif /* ARC_OPTS_H */

I wonder where the vale 0x1000 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 0x1000 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 = _cpu_types[(int) arc_cpu];
> +  arc_selected_arch = _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. 

RE: [PATCH] [ARC] New option handling, refurbish multilib support.

[Claudiu Zissulescu]

Hi Andrew,

> Sorry it's taken s long to review this patch.
> 
This is understandable, it is a quite large patch and changes a lot of items.

> In general I like this, and think it's a step in the right direction.
> I have a few pretty minor concerns, but hopefully nothing too
> contentious.
> 
> In general I like the use of the *.def files, but my only concern is
> the lack of explanation in any of them about what they're for.  It
> would be nice if each had a summary of what each field represents to
> make it easier to add new entries.

This is a good point, I will add extra info on how to add new entries.

> 
> The use of arc_seen_options can, I think be replaced by using
> global_opts_seen.x_target_flags, or maybe there's a detail I'm not
> understanding, in which case maybe a comment somewhere to explain why
> those two things are different.

I cannot remember why I haven't use the global_options_set, but I will try as u 
suggested.

> 
> The only thing I dislike in this patch is the switch from 'arc_cpu' to
> a set of global boolean flags.  I can't see why we'd ever want more
> than one of those architecture flags to be true, so I'd rather we
> switched back to an enum if that's possible.
> 
I remember, initially I wanted to use enums, but I bumped into an issue, don't 
remember exactly what.  As far as I can see now, it perfectly make sense your 
comment. 

I will come back to u with extra info/revised patch asap,
Claudiu

Re: [PATCH] [ARC] New option handling, refurbish multilib support.

[Andrew Burgess]

* Claudiu Zissulescu  [2016-10-10 15:26:47 
+0200]:

> Hi Andrew,
> 
> This is updated patch of the original sent to mailing list some while ago.
> 
> What is new:
>  - Do not use MULTILIB_REUSE as its semantic changed, and the old one was 
> causing issues while building.
>  - Update invoke.texi documentation adding nps400 option to mcpu.
> 
> This patch is important as it changes the way how we handle CPU
> variations and multilib support. It will be great if you can include
> this patch on your review list before any other one.

Sorry it's taken s long to review this patch.

In general I like this, and think it's a step in the right direction.
I have a few pretty minor concerns, but hopefully nothing too
contentious.

In general I like the use of the *.def files, but my only concern is
the lack of explanation in any of them about what they're for.  It
would be nice if each had a summary of what each field represents to
make it easier to add new entries.

The use of arc_seen_options can, I think be replaced by using
global_opts_seen.x_target_flags, or maybe there's a detail I'm not
understanding, in which case maybe a comment somewhere to explain why
those two things are different.

The only thing I dislike in this patch is the switch from 'arc_cpu' to
a set of global boolean flags.  I can't see why we'd ever want more
than one of those architecture flags to be true, so I'd rather we
switched back to an enum if that's possible.

[ I've left my review comments in line below too, but the above is a
summary of everything, there's nothing additional below. ]

Thanks,
Andrew

> 
> Thanks,
> Claudiu
> 
> gcc/
> 2016-05-09  Claudiu Zissulescu  
> 
>   * config/arc/arc-arch.h: New file.
>   * config/arc/arc-arches.def: Likewise.
>   * config/arc/arc-cpus.def: Likewise.
>   * config/arc/arc-options.def: Likewise.
>   * config/arc/t-multilib: Likewise.
>   * config/arc/genmultilib.awk: Likewise.
>   * config/arc/genoptions.awk: Likewise.
>   * config/arc/arc-tables.opt: Likewise.
>   * config/arc/driver-arc.c: Likewise.
>   * common/config/arc/arc-common.c (arc_handle_option): Trace
>   toggled options.
>   * config.gcc (arc*-*-*): Add arc-tables.opt to arc's extra
>   options; check for supported cpu against arc-cpus.def file.
>   (arc*-*-elf*, arc*-*-linux-uclibc*): Use new make fragment; define
>   TARGET_CPU_BUILD macro; add driver-arc.o as an extra object.
>   * config/arc/arc-c.def: Add emacs local variables.
>   * config/arc/arc-opts.h (processor_type): Use arc-cpus.def file.
>   (FPU_FPUS, FPU_FPUD, FPU_FPUDA, FPU_FPUDA_DIV, FPU_FPUDA_FMA)
>   (FPU_FPUDA_ALL, FPU_FPUS_DIV, FPU_FPUS_FMA, FPU_FPUS_ALL)
>   (FPU_FPUD_DIV, FPU_FPUD_FMA, FPU_FPUD_ALL): New defines.
>   (DEFAULT_arc_fpu_build): Define.
>   (DEFAULT_arc_mpy_option): Define.
>   * config/arc/arc-protos.h (arc_init): Delete.
>   * config/arc/arc.c (arc_cpu_name): New variable.
>   (arc_selected_cpu, arc_selected_arch, arc_arcem, arc_archs)
>   (arc_arc700, arc_arc600, arc_arc601): New variable.
>   (arc_init): Add static; remove selection of default tune value,
>   cleanup obsolete error messages.
>   (arc_override_options): Make use of .def files for selecting the
>   right cpu and option configurations.
>   * config/arc/arc.h (stdbool.h): Include.
>   (TARGET_CPU_DEFAULT): Define.
>   (CPP_SPEC): Remove mcpu=NPS400 handling.
>   (arc_cpu_to_as): Declare.
>   (EXTRA_SPEC_FUNCTIONS): Define.
>   (OPTION_DEFAULT_SPECS): Likewise.
>   (ASM_DEFAULT): Remove.
>   (ASM_SPEC): Use arc_cpu_to_as.
>   (DRIVER_SELF_SPECS): Remove deprecated options.
>   (arc_arc700, arc_arc600, arc_arc601, arc_arcem, arc_archs):
>   Declare.
>   (TARGET_ARC600, TARGET_ARC601, TARGET_ARC700, TARGET_EM)
>   (TARGET_HS, TARGET_V2, TARGET_ARC600): Make them use arc_arc*
>   variables.
>   (MULTILIB_DEFAULTS): Use ARC_MULTILIB_CPU_DEFAULT.
>   * config/arc/arc.md (attr_cpu): Remove.
>   * config/arc/arc.opt (arc_mpy_option): Make it target variable.
>   (mno-mpy): Deprecate.
>   (mcpu=ARC600, mcpu=ARC601, mcpu=ARC700, mcpu=NPS400, mcpu=ARCEM)
>   (mcpu=ARCHS): Remove.
>   (mcrc, mdsp-packa, mdvbf, mmac-d16, mmac-24, mtelephony, mrtsc):
>   Deprecate.
>   (mbarrel_shifte, mspfp_, mdpfp_, mdsp_pack, mmac_): Remove.
>   (arc_fpu): Use new defines.
>   (arc_seen_options): New target variable.
>   * config/arc/t-arc (driver-arc.o): New target.
>   (arc-cpus, t-multilib, arc-tables.opt): Likewise.
>   * config/arc/t-arc-newlib: Delete.
>   * config/arc/t-arc-uClibc: Renamed to t-uClibc.
>   * doc/invoke.texi (ARC): Update arc options.
> ---
>  gcc/common/config/arc/arc-common.c | 162 -
>  gcc/config.gcc |  47 +
>