Re: [PATCH] gas/arc: Allow --with-cpu configure option to change default cpu

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

Nick,

Thanks for the feedback.  My thoughts are inline...

* Nick Clifton <nickc@redhat.com> [2016-06-14 13:18:41 +0100]:

> Hi Andrew,
> 
>   (Sorry for the delay in reviewing this patch).
> 
> > The solution I propose is to add a --with-cpu=NAME configure option (a
> > similar flag is already used by other targets in GCC) that sets up a
> > default value in config.h, this default is then used in
> > config/tc-arc.c if the user of the built tools does not explicitly
> > select a cpu type.
> 
> This makes sense, but there is a caveat:
> 
> > If there's a better solution, or if I've gone wrong with the autotools
> > side of things, then all suggestions for improvements are welcome.
> 
> Your current implementation, I believe, adds the -with-cpu option as a 
> general configure option.  Which means that other, non-ARC users, are going
> to start complaining that it does not work for their particular architecture.
> (Unless of course you would like to propose a giant patch that does 
> implement --with-cpu for all target architectures).
> 
> Instead, I think that following the direction of the NDS32 port would be
> better.  This implements a --with-arch= option, but only if the target is
> the NDS32.  (See gas/configure.ac around line 436).  You could add --with-cpu
> support for the ARC in a similar fashion.

I see your concern here, however...

If I move the option checking into ARC specific code, but still use
AC_ARG_WITH to check for the argument then the help text will always
show up for all targets.  Due to our use of AC_DISABLE_OPTION_CHECKING
all --with-XXX type options will be accepted by our configure script,
and as you point out, when this does not work this might confuse
users.

This would suggest that I should stop using AC_ARG_WITH in the ARC
specific code to check for the --with-cpu option.  This is what NDS32
does, and obviously, I could get this working, however I'd like to
make an alternative suggestion.

What if I changed the current patch to simply remove the help text for
the --with-cpu option?  Probably along with adding a comment in the
configure script to say that not all targets support this option,
please check the source code.

Under both schemes --with-cpu will be accepted (thanks to
AC_DISABLE_OPTION_CHECKING).

Under both schemes --with-cpu does not appear in the help text, so
users are not confused.

The benefit as I see it is that if another target wants to support
this feature then --with-cpu is already in place, and is the natural
choice to use.

I think that this is a good thing, as --with-cpu is what GCC uses for
similar functionality, and though (obviously) it is not a requirement
for us to use the same flag names as GCC, I can't help but think this
would be a good thing.

> 
> There is also the question of where to document this feature.  The gas/README,
> gas/NEWS and gas/doc/c-arc.texi files spring to mind.

I took a look.  NEWS seems like an obvious choice.  The doc/c-arc.texi
does not seem to mention any other configure time options, nor could I
find such much in any other targets manuals.  This felt like a bit too
target specific to be adding to the README file.

I put together a new patch based on the above idea of leaving the
option global but removing it from the configure help text.  Let me
know what you think.  If you're still not happy I can move the option
into ARC specific code.

Thanks,
Andrew

---

gas/arc: Allow --with-cpu configure option to change default cpu

Add a new configure time option --with-cpu=CPU that can be used to
change the default cpu selection.  Currently this is only supported on
ARC.

If the user specifies --with-cpu=FOO at configure time then the constant
TARGET_WITH_CPU will be defined to "FOO" in config.h, the
TARGET_WITH_CPU constant can then be used in the config/tc-*.c code as a
default string to select an appropriate cpu if the user does not
explicitly select a cpu at gas run-time.

gas/ChangeLog:

	* config.in (TARGET_WITH_CPU): Undefine.
	* configure.ac: Add --with-cpu support, and define in config.h.
	* configure: Regenerate.
	* config/tc-arc.c: Use TARGET_WITH_CPU to select default CPU.
	* NEWS: Mention new configure option.
---
 gas/ChangeLog       |  8 ++++++++
 gas/NEWS            |  3 +++
 gas/config.in       |  3 +++
 gas/config/tc-arc.c |  6 +++++-
 gas/configure       | 18 ++++++++++++++++--
 gas/configure.ac    |  7 +++++++
 6 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/gas/NEWS b/gas/NEWS
index 08807f1..d48b097 100644
--- a/gas/NEWS
+++ b/gas/NEWS
@@ -32,6 +32,9 @@
 
 * Add assembly-time relaxation option for ARC cpus.
 
+* Add --with-cpu=TYPE configure option for ARC gas.  This allows the default
+  cpu type to be adjusted at configure time.
+
 Changes in 2.26:
 
 * Add a configure option --enable-compressed-debug-sections={all,gas} to
diff --git a/gas/config.in b/gas/config.in
index e06f160..5129c28 100644
--- a/gas/config.in
+++ b/gas/config.in
@@ -318,6 +318,9 @@
 /* Target vendor. */
 #undef TARGET_VENDOR
 
+/* Target specific CPU. */
+#undef TARGET_WITH_CPU
+
 /* Use b modifier when opening binary files? */
 #undef USE_BINARY_FOPEN
 
diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 155d991..d46f593 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -51,6 +51,10 @@
 /* Equal to MAX_PRECISION in atof-ieee.c.  */
 #define MAX_LITTLENUMS 6
 
+#ifndef TARGET_WITH_CPU
+#define TARGET_WITH_CPU "arc700"
+#endif /* TARGET_WITH_CPU */
+
 /* Enum used to enumerate the relaxable ins operands.  */
 enum rlx_operand_type
 {
@@ -2466,7 +2470,7 @@ md_begin (void)
   const struct arc_opcode *opcode = arc_opcodes;
 
   if (!mach_type_specified_p)
-    arc_select_cpu ("arc700");
+    arc_select_cpu (TARGET_WITH_CPU);
 
   /* The endianness can be chosen "at the factory".  */
   target_big_endian = byte_order == BIG_ENDIAN;
diff --git a/gas/configure b/gas/configure
index b6298b5..df350b2 100755
--- a/gas/configure
+++ b/gas/configure
@@ -771,6 +771,7 @@ enable_x86_relax_relocations
 enable_elf_stt_common
 enable_werror
 enable_build_warnings
+with_cpu
 enable_nls
 enable_maintainer_mode
 with_system_zlib
@@ -1435,6 +1436,7 @@ Optional Packages:
   --with-pic              try to use only PIC/non-PIC objects [default=use
                           both]
   --with-gnu-ld           assume the C compiler uses GNU ld [default=no]
+
   --with-system-zlib      use installed libz
 
 Some influential environment variables:
@@ -10982,7 +10984,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 10985 "configure"
+#line 10987 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -11088,7 +11090,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11091 "configure"
+#line 11093 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -11828,6 +11830,13 @@ fi
 ac_config_headers="$ac_config_headers config.h:config.in"
 
 
+
+# Check whether --with-cpu was given.
+if test "${with_cpu+set}" = set; then :
+  withval=$with_cpu;
+fi
+
+
 # PR 14072
 
 
@@ -12870,6 +12879,11 @@ cat >>confdefs.h <<_ACEOF
 _ACEOF
 
 
+cat >>confdefs.h <<_ACEOF
+#define TARGET_WITH_CPU "${with_cpu}"
+_ACEOF
+
+
 for ac_prog in 'bison -y' byacc
 do
   # Extract the first word of "$ac_prog", so it can be a program name with args.
diff --git a/gas/configure.ac b/gas/configure.ac
index 8f71825..c6116c5 100644
--- a/gas/configure.ac
+++ b/gas/configure.ac
@@ -106,6 +106,12 @@ AM_BINUTILS_WARNINGS
 # Generate a header file
 AC_CONFIG_HEADERS(config.h:config.in)
 
+dnl Option --with-cpu=TYPE allows configure type control of the default
+dnl cpu type within the assembler.  Not all targets support this option
+dnl yet, which is why there is no help string, check the code to see if
+dnl your target supports this option.
+AC_ARG_WITH(cpu, [], [],[])
+
 # PR 14072
 AH_VERBATIM([00_CONFIG_H_CHECK],
 [/* Check that config.h is #included before system headers
@@ -780,6 +786,7 @@ AC_DEFINE_UNQUOTED(TARGET_CANONICAL,	"${target}",       [Canonical target.])
 AC_DEFINE_UNQUOTED(TARGET_CPU,		"${target_cpu}",   [Target CPU.])
 AC_DEFINE_UNQUOTED(TARGET_VENDOR,	"${target_vendor}", [Target vendor.])
 AC_DEFINE_UNQUOTED(TARGET_OS,		"${target_os}",    [Target OS.])
+AC_DEFINE_UNQUOTED(TARGET_WITH_CPU,	"${with_cpu}",     [Target specific CPU.])
 
 AC_PROG_YACC
 AM_PROG_LEX
-- 
2.5.1