Hi,
Thanks for the feedback. I respond here to the remaining issues:
> > 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.)
Support for __float128 has been removed.
Fixed in:
r189655 | sofiane | 2012-07-19 13:24:57 +0100 (Thu, 19 Jul 2012) | 19 lines
[AArch64] Remove __float128 support.
> > +@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?
Documentation formatting has been fixed to conform to the required styling.
Also the documentation has been updated to clarify ambiguous parts or add
missing ones.
Fixed in:
r188895 | belagod | 2012-06-22 18:23:05 +0100 (Fri, 22 Jun 2012) | 11 lines
[AArch64] Fix documentation layout.
> > +@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.
Same as above.
Fixed in:
r188895 | belagod | 2012-06-22 18:23:05 +0100 (Fri, 22 Jun 2012) | 11 lines
[AArch64] Fix documentation layout.
> > +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?
Same as above.
Fixed in:
r188895 | belagod | 2012-06-22 18:23:05 +0100 (Fri, 22 Jun 2012) | 11 lines
[AArch64] Fix documentation layout.
> > + 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.)
sprinsf has been replaced with snprintf and sizeof (templ) as appropriate.
Fixed in:
r188896 | belagod | 2012-06-22 18:32:35 +0100 (Fri, 22 Jun 2012) | 13 lines
[AArch64] Replace sprintf with snprintf.
> > +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.
Handling of this option has been rewritten to use the Enum generic option
handling framework.
Fixed in:
r188894 | belagod | 2012-06-22 18:12:39 +0100 (Fri, 22 Jun 2012) | 28 lines
[AArch64] Use Enums for code models option selection.
> > +mtls-dialect=
> > +Target RejectNegative Joined Var(aarch64_tls_dialect_string)
> > +Use given thread-local storage dialect
>
> Likewise.
As above.
Fixed in:
r188892 | belagod | 2012-06-22 17:55:05 +0100 (Fri, 22 Jun 2012) | 18 lines
[AArch64] Use Enums for TLS option selection.
> > + 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.
A testcase has been added for the above diagnostic.
Fixed in:
r190481 | sofiane | 2012-08-17 15:53:50 +0100 (Fri, 17 Aug 2012) | 11 lines
[AArch64] Add new testcases to improve diagnostics.
> 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.
Error messages have been reviewed throughout and fixed/improved as
suggested.
Fixed in:
r189653 | sofiane | 2012-07-19 12:59:07 +0100 (Thu, 19 Jul 2012) | 12 lines
[AArch64] Improve diagnostics.
> > +#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.
Fixed as suggested.
Fixed in:
r188897 | belagod | 2012-06-22 18:42:12 +0100 (Fri, 22 Jun 2012) | 6 lines
[AArch64] Update LINK_SPEC.
> > @@ -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?
Fixed as suggested.
Fixed in:
r188331 | mshawcroft | 2012-06-08 08:23:45 +0100 (Fri, 08 Jun 2012) | 7
lines
> > +#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.
The file with the above no longer exists.
Fixed in:
r188329 | mshawcroft | 2012-06-08 08:10:08 +0100 (Fri, 08 Jun 2012) | 11
lines
-----
Thanks
Sofiane
