General question: I suppose you expect to submit patches soon for
other toolchain components (such as binutils, GDB, glibc) and the
Linux kernel, if you haven't done so yet?
> Index: config.guess
> ===================================================================
> --- config.guess (revision 187870)
> +++ config.guess (working copy)
> @@ -4,7 +4,7 @@
> # 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
> # 2011 Free Software Foundation, Inc.
>
> -timestamp='2011-06-03'
> +timestamp='2012-05-25'
Instead of patching config.sub or config.guess, you should copy the
latest version, unmodified, from upstream config.git.
> Index: gcc/doc/extend.texi
> ===================================================================
> --- gcc/doc/extend.texi (revision 187870)
> +++ gcc/doc/extend.texi (working copy)
> @@ -935,7 +935,8 @@
>
> Not all targets support additional floating point types. @code{__float80}
> and @code{__float128} types are supported on i386, x86_64 and ia64 targets.
> -The @code{__float128} type is supported on hppa HP-UX targets.
> +The @code{__float128} type is supported on hppa HP-UX targets and ARM AArch64
> +targets.
I don't see any good reason to support it on AArch64, since it's the
same as "long double" there. (It's on PA HP-UX as a workaround for
libquadmath requiring the type rather than being able to with with a
type called either "long double" or "__float128" - libquadmath being
used on PA HP-UX as a workaround for the system libm lacking much long
double support. But that shouldn't be an issue for new targets such
as AArch64 GNU/Linux. And my understanding from N1582 is that the C
bindings for IEEE 754-2008, being worked on for a five-part ISO/IEC
TS, are expected to use names such as _Float128, not __float128, as
standard names for supported IEEE floating-point types.)
> +@opindex mbig-endian
> +Generate big-endian code. This is the default when GCC is configured for an
> +@samp{aarch64*be-*-*} target.
In general, throughout Texinfo changes, two spaces after "." at the
end of a sentence.
> +@item -march=@var{name}
> +@opindex march
> +Specify the name of the target architecture, optionally suffixed by one or
> +more feature modifiers. This option has the form
> +@samp{-march=<arch>[+[no]<feature>]}, where the only value for @samp{<arch>}
> +is @samp{armv8}, and the possible values for @samp{<feature>} are
> +@samp{crypto}, @samp{fp}, @samp{simd}.
It's unfortunate that you've chosen this complicated syntax that means
the generic support for enumerated option arguments cannot be used
(and so --help information cannot list supported CPUs and features).
A simpler syntax where -march takes just an architecture name and
features have separate options would seem better, and more in line
with most other architectures supported by GCC.
There are several Texinfo problems above. Instead of <feature> you
should use @var{feature}, and since the '[' and ']' are not literal
text they should be inside @r{} - the proper way of writing
@samp{-march=<arch>[+[no]<feature>]} would be
@option{-march=@var{arch}@r{[}+@r{[}no@r{]}@var{feature}@r{]}}.
Also, could you document what the feature names mean?
> +@item -mcpu=@var{name}
> +@opindex mcpu
> +Specify the name of the target processor, optionally suffixed by one or more
> +feature modifiers. This option has the form
> @samp{-cpu=<cpu>[+[no]<feature>]},
> +where the possible values for @samp{<cpu>} are @samp{generic}, @samp{large},
> +and the possible values for @samp{<feature>} are @samp{crypto}, @samp{fp},
> +@samp{simd}.
Same comments apply.
> +This option is very similar to the -mcpu= option, except that instead of
@option{-mcpu=}. And does -mtune= take feature names or just plain CPU
names?
> + if (mvn == 0)
> + {
> + if (widthc != 'd')
> + sprintf (templ,"movi\t%%0.%d%c, %%1, lsl %d ",(64/width),
> + widthc, shift);
> + else
> + sprintf (templ,"movi\t%%d0, %%1");
> + }
> + else
> + sprintf (templ,"mvni\t%%0.%d%c, %%1, lsl %d",(64/width),
> + widthc, shift);
Presumably you have some logic for why the 40-byte buffer size is
enough, but could you use snprintf with sizeof (templ) specified in
the call to protect against any mistakes in that logic? Also, spaces
after commas and around the "/" in the division, and the second line
in the function call should be lined up immediately after the opening
'(', not further right. (Check for and fix all these issues elsewhere
in the port as well; I've just pointed out a representative instance
of them.)
> +__extension__ static __inline int8x8_t __attribute__ ((__always_inline__))
> +vneg_s8 (int8x8_t a)
> +{
> + int8x8_t result;
> + __asm__ ("neg %0.8b,%1.8b"
> + : "=w"(result)
> + : "w"(a)
> + : /* No clobbers */);
> + return result;
> +}
I presume you've determined for each intrinsic represented as an asm
that it cannot usefully be described using generic GNU C, GIMPLE or
RTL - as an example, for this one, what are its semantics that generic
internal representations cannot handle usefully?
Are you sure that the identifiers "a", "b" and "result" are supposed
to be reserved by this header, so users cannot define them as macros,
or should it actually be using __a, __b and __result?
> + __extension__ \
> + ({ \
> + int16x8_t b_ = (b); \
> + int8x8_t a_ = (a); \
Here "a_" and "b_" seem to be more surprising reservations.
> +__extension__ static __inline int32x4_t __attribute__ ((__always_inline__))
> +vsubhn_high_s64 (int32x2_t a, int64x2_t b, int64x2_t c)
"c" seems a surprising reservation.
> + vld3 ## Q ## _ ## funcsuffix (const ptrtype *ptr) \
"ptr" seems a surprising reservation.
In general, perhaps this header could do with comments explaining the
rationale for the choice between inline functions with asms, macros
with asms, built-in functions, etc., so people can understand why the
different intrinsics have been implemented in the way they have.
> +mcmodel=
> +Target RejectNegative Joined Var(aarch64_mem_model_string)
> +Specify the PIC memory model - tiny, small, compact, medium, large
Even if you don't want to change the -march/-mcpu syntax, this at
least ought to be able to use Enum to have the generic option handling
machinery parse the different possible arguments.
> +mtls-dialect=
> +Target RejectNegative Joined Var(aarch64_tls_dialect_string)
> +Use given thread-local storage dialect
Likewise.
> + case SIMD_ARG_CONSTANT:
> + if (!(*insn_data[icode].operand[argc + have_retval].predicate)
> + (op[argc], mode[argc]))
> + error ("incompatible type for argument %d, "
> + "expected 'const int'", argc + 1);
I don't see a testcase for this diagnostic in the testsuite patch;
each diagnostic should have a testcase.
Use %<const int%> instead of literal ''.
Is it possible to get a location here and use error_at? The
diagnostic functions using input_location implicitly are deprecated,
and as I understand this is called during built-in function expansion,
when input_location is likely to be particularly unhelpful.
> + default:
> + error ("Unimplemented memory model in generating symbolic references.
> */");
> + gcc_unreachable ();
Diagnostics should start with a lowercase letter, and not end with a
period (and certainly not end with ". */"). As I understand it, this
is an internal error, not diagnosing a problem in the user's code, so
should actually use internal_error.
> + error ("missing architectural extension");
What does this error mean? Can you pass in a location and use
error_at? Also add a test for it to the testsuite.
> + /* Extension not found in list. */
> + error ("unknown architectural extension `%s'", str);
Again, test in the testsuite and use a location if possible. Use %qs
not `%s'; the GNU Coding Standards no longer recommend `' for quoting.
> + error ("missing arch name `%s'\n", str);
Same comments. No '\n' at end of diagnostics; it's generated
automatically.
> + /* ARCH name not found in list. */
> + error ("unknown value (%s) for -march", str);
Same comments; using %qs instead of (%s) is more consistent.
> + error ("missing cpu name in `%s'", str);
Same comments.
> + error ("unknown value (%s) for -mcpu", str);
Same comments.
> + error ("unknown value (%s) for -mtune", str);
Same comments. Remember that if you used a finite set of enumerated
arguments for each option, you wouldn't need any of this code at
all....
> + if (aarch64_tls_dialect_string)
> + {
> + if (strcmp (aarch64_tls_dialect_string, "traditional") == 0)
> + aarch64_tls_dialect = TLS_DIALECT_TRADITIONAL;
> + else if (strcmp (aarch64_tls_dialect_string, "desc") == 0)
> + aarch64_tls_dialect = TLS_DIALECT_DESC;
> + else
> + error ("bad value (%s) for -mtls-dialect",
> aarch64_tls_dialect_string);
Same comment; here you can clearly use Enum in the .opt file.
> + if (aarch64_default_mem_model == AARCH64_MEM_MODEL_UNIMPLEMENTED)
> + error ("Unimplemented / Illegal memory model `%s'%s",
> + aarch64_mem_model_string,
> + flag_pic ? " with -fPIC" : "");
Same comments. In addition, the GNU Coding Standards say that
"illegal" is only used for violations of law; errors in usage of a
program should use "invalid". You can't add " with -fPIC" as a
fragment like that as the full sentence needs to be in gcc.pot for
translators. That is, you should do something like
if (flag_pic)
error ("unimplemented or invalid memory model %qs with -fPIC", ...);
else
error (full sentence again);
(but error_at with a location is better).
> +/* Ensure OPERAND lies between LOW (inclusive) and HIGH (exclusive). Raise
> + ERR if it doesn't. */
> +static void
> +bounds_check (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
> + const char *err)
> +{
> + HOST_WIDE_INT lane;
> +
> + gcc_assert (GET_CODE (operand) == CONST_INT);
> +
> + lane = INTVAL (operand);
> +
> + if (lane < low || lane >= high)
> + error ("%s", err);
Again, actual message text should be passed to error functions.
Instead of passing around a string, pass an enum for the possible
error conditions, and have a switch statement here choosing between a
number of possible calls to error_at.
> +#define MULTIARCH_TUPLE "aarch64-linux-gnu"
> +
> +#undef STANDARD_STARTFILE_PREFIX_1
> +#define STANDARD_STARTFILE_PREFIX_1 "/lib/" MULTIARCH_TUPLE "/"
> +
> +#undef STANDARD_STARTFILE_PREFIX_2
> +#define STANDARD_STARTFILE_PREFIX_2 "/usr/lib/" MULTIARCH_TUPLE "/"
This shouldn't be here unconditionally. Once we've got Matthias's
Debian multiarch support in, things should be conditional for AArch64
in whatever way they are for other architectures.
> +#define LINUX_TARGET_LINK_SPEC "%{h*} %{version:-v} \
I removed %{version:-v} specs in
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00892.html>; don't add
them back.
> + %{b} \
I removed %{b} specs in
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00197.html> and
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00890.html>; don't add
them back.
> + %{!dynamic-linker:-dynamic-linker " GNU_USER_DYNAMIC_LINKER "} \
I removed %{!dynamic-linker:} uses in
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00194.html>; don't add
them back.
> @@ -278,6 +281,18 @@
> esac
>
> case ${host} in
> +aarch64*-*-elf)
> + extra_parts="$extra_parts crtbegin.o crtend.o crti.o crtn.o"
> + tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
> + tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
> + tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
> + ;;
> +aarch64*-*-linux*)
> + md_unwind_header=aarch64/linux-unwind.h
> + tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
> + tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
> + tmake_file="${tmake_file} ${cpu_type}/t-linux"
> + ;;
Wht are you using t-softfp-sfdf and t-softfp-excl when
aarch64/t-softfp appears to override them completely?
> +#ifdef __ELF__
> +#define TYPE(x) .type SYM(x),function
> +#define SIZE(x) .size SYM(x), . - SYM(x)
> +#define LSYM(x) .x
Are you really planning to support non-ELF for AArch64? If no, leave
out the conditionals.
> +#endif
> +#endif
> \ No newline at end of file
Add the missing newline.
--
Joseph S. Myers
jos...@codesourcery.com
