On Thu, Jun 11, 2015 at 12:28:08PM +0100, Alan Lawrence wrote:
> Hi,
>
> This is a follow-up to Jim Wilson's patch fixing ICE's with
> -march=armv8-a+nofp, and the discussion here:
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00177.html
>
> The first patch improves the error messages to describe what kind of code
> caused the problem, and to error rather than "sorry" (we should not be sorry,
> the user has asked the compiler to do something that makes no sense!).
> Moreover, to issue an error, rather than ICE, on some testcases (supplied!).
>
> The error messages in aarch64_setup_incoming_varargs and
> aarch64_expand_builtin_va_start are then never reached as error() has already
> been called, so change them to asserts.
>
> Compiling with -mgeneral-regs-only on functions taking vector arguments, and
> simple arithmetic using float/double, all raise the error; on functions using
> vectors only internally not via ABI, at least some are handled by the midend
> using scalar code.
>
>
> The second patch cleans up the documentation in line with the previous
> discussion.
Submissions on this list should be one patch per mail, it makes
tracking review easier.
Comments inline below.
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-protos.h (aarch64_err_no_fpadvsimd): New.
>
> * config/aarch64/aarch64.md (mov<mode>/GPF, movtf): Use
> aarch64_err_no_fpadvsimd.
>
> * config/aarch64/aarch64.c (aarch64_err_no_fpadvsimd): New.
> (aarch64_layout_arg, aarch64_init_cumulative_args): Use
> aarch64_err_no_fpadvsimd if !TARGET_FLOAT and we need FP regs.
> (aarch64_expand_builtin_va_start, aarch64_setup_incoming_varargs):
> Turn error into assert, test TARGET_FLOAT.
> (aarch64_gimplify_va_arg_expr): Use aarch64_err_no_fpadvsimd, test
> TARGET_FLOAT.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/mgeneral-regs_1.c: New file.
> * gcc.target/aarch64/mgeneral-regs_2.c: New file.
> * gcc.target/aarch64/nofp_1.c: New file.
>
> gcc/ChangeLog:
>
> * doc/invoke.texi: Clarify AArch64 feature modifiers (no)fp, (no)simd
> and (no)crypto.
> commit efbf0f4699ac963472834c912b46b1a3a076fa64
> Author: Alan Lawrence <alan.lawre...@arm.com>
> Date: Mon Jan 12 15:04:06 2015 +0000
>
> Approved r/3008, rebased
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 965a11b..ac92c59 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -259,6 +259,7 @@ unsigned aarch64_dbx_register_number (unsigned);
> unsigned aarch64_trampoline_size (void);
> void aarch64_asm_output_labelref (FILE *, const char *);
> void aarch64_elf_asm_named_section (const char *, unsigned, tree);
> +void aarch64_err_no_fpadvsimd (machine_mode, const char *);
> void aarch64_expand_epilogue (bool);
> void aarch64_expand_mov_immediate (rtx, rtx);
> void aarch64_expand_prologue (void);
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 968a6b6..5636e7b 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -534,6 +534,16 @@ static const char * const aarch64_condition_codes[] =
> "hi", "ls", "ge", "lt", "gt", "le", "al", "nv"
> };
>
> +void
> +aarch64_err_no_fpadvsimd (machine_mode mode, const char *msg)
> +{
> + const char *mc = FLOAT_MODE_P (mode) ? "floating point" : "vector";
GCC coding conventions, this should be
floating-point (https://gcc.gnu.org/codingconventions.html).
> + if (TARGET_GENERAL_REGS_ONLY)
> + error ("%qs is incompatible with %s %s", "-mgeneral-regs-only", mc, msg);
> + else
> + error ("%qs feature modifier is incompatible with %s %s", "+nofp", mc,
> msg);
> +}
> +
> static unsigned int
> aarch64_min_divisions_for_recip_mul (enum machine_mode mode)
> {
> @@ -1784,6 +1794,9 @@ aarch64_layout_arg (cumulative_args_t pcum_v,
> machine_mode mode,
> and homogenous short-vector aggregates (HVA). */
> if (allocate_nvrn)
> {
> + if (!TARGET_FLOAT)
> + aarch64_err_no_fpadvsimd (mode, "argument");
> +
> if (nvrn + nregs <= NUM_FP_ARG_REGS)
> {
> pcum->aapcs_nextnvrn = nvrn + nregs;
> @@ -1910,6 +1923,17 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS *pcum,
> pcum->aapcs_stack_words = 0;
> pcum->aapcs_stack_size = 0;
>
> + if (!TARGET_FLOAT
> + && fndecl && TREE_PUBLIC (fndecl)
> + && fntype && fntype != error_mark_node)
> + {
> + const_tree args, type = TREE_TYPE (fntype);
> + machine_mode mode; /* To pass pointer as argument; never used. */
> + int nregs; /* Likewise. */
Do these need annotations to avoid errors in a Werror build? I don't see
any mention of what testing this patch has been through?
> + if (aarch64_vfp_is_call_or_return_candidate (TYPE_MODE (type), type,
> + &mode, &nregs, NULL))
> + aarch64_err_no_fpadvsimd (TYPE_MODE (type), "return type");
> + }
> return;
> }
>
> @@ -7573,9 +7597,7 @@ aarch64_expand_builtin_va_start (tree valist, rtx
> nextarg ATTRIBUTE_UNUSED)
>
> if (!TARGET_FLOAT)
> {
> - if (cum->aapcs_nvrn > 0)
> - sorry ("%qs and floating point or vector arguments",
> - "-mgeneral-regs-only");
> + gcc_assert (cum->aapcs_nvrn == 0);
This promotes an error to an ICE? Can we really never get to this
point through the error control flow I would have expected that to
report an error then return control-flow to here.
> vr_save_area_size = 0;
> }
>
> @@ -7682,8 +7704,7 @@ aarch64_gimplify_va_arg_expr (tree valist, tree type,
> gimple_seq *pre_p,
> {
> /* TYPE passed in fp/simd registers. */
> if (!TARGET_FLOAT)
> - sorry ("%qs and floating point or vector arguments",
> - "-mgeneral-regs-only");
> + aarch64_err_no_fpadvsimd (mode, "varargs");
>
> f_top = build3 (COMPONENT_REF, TREE_TYPE (f_vrtop),
> unshare_expr (valist), f_vrtop, NULL_TREE);
> @@ -7920,9 +7941,7 @@ aarch64_setup_incoming_varargs (cumulative_args_t
> cum_v, machine_mode mode,
>
> if (!TARGET_FLOAT)
> {
> - if (local_cum.aapcs_nvrn > 0)
> - sorry ("%qs and floating point or vector arguments",
> - "-mgeneral-regs-only");
> + gcc_assert (local_cum.aapcs_nvrn == 0);
As above?
> vr_saved = 0;
> }
>
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 11123d6..99cefec 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -981,10 +981,7 @@
> ""
> "
> if (!TARGET_FLOAT)
> - {
> - sorry (\"%qs and floating point code\", \"-mgeneral-regs-only\");
> - FAIL;
> - }
> + aarch64_err_no_fpadvsimd (<MODE>mode, \"code\");
You've dropped the FAIL?
>
> if (GET_CODE (operands[0]) == MEM)
> operands[1] = force_reg (<MODE>mode, operands[1]);
> @@ -1035,10 +1032,7 @@
> ""
> "
> if (!TARGET_FLOAT)
> - {
> - sorry (\"%qs and floating point code\", \"-mgeneral-regs-only\");
> - FAIL;
> - }
> + aarch64_err_no_fpadvsimd (TFmode, \"code\");
Likewise.
>
> if (GET_CODE (operands[0]) == MEM)
> operands[1] = force_reg (TFmode, operands[1]);
> diff --git a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c
> b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c
> new file mode 100644
> index 0000000..b5192a6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c
> @@ -0,0 +1,10 @@
> +/* { dg-options "-mgeneral-regs-only" } */
> +
> +typedef int int32x2_t __attribute__ ((__vector_size__ ((8))));
> +
> +/* { dg-error "'-mgeneral-regs-only' is incompatible with vector return
> type" "" {target "aarch64*-*-*"} 7 } */
> +/* { dg-error "'-mgeneral-regs-only' is incompatible with vector argument"
> "" {target "aarch64*-*-*"} 7 } */
> +int32x2_t test (int32x2_t a, int32x2_t b)
> +{
> + return a + b;
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_2.c
> b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_2.c
> new file mode 100644
> index 0000000..8590199
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_2.c
> @@ -0,0 +1,15 @@
> +/* { dg-options "-mgeneral-regs-only" } */
> +
> +#include <stdarg.h>
> +
> +typedef int int32x2_t __attribute__ ((__vector_size__ ((8))));
> +
> +int
> +test (int i, ...)
> +{
> + va_list argp;
> + va_start (argp, i);
> + int32x2_t x = (int32x2_t) {0, 1};
> + x += va_arg (argp, int32x2_t); /* { dg-error "'-mgeneral-regs-only' is
> incompatible with vector varargs" } */
> + return x[0] + x[1];
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/nofp_1.c
> b/gcc/testsuite/gcc.target/aarch64/nofp_1.c
> new file mode 100644
> index 0000000..3fc0036
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/nofp_1.c
> @@ -0,0 +1,19 @@
> +/* { dg-skip-if "conflicting -march" { *-*-* } { "-march=*" } {
> "-march=*+nofp" } } */
> +/* If there are multiple -march's, the latest wins; skip the test either way.
> + -march overrides -mcpu, so there is no possibility of conflict. */
> +
> +/* { dg-options "-march=armv8-a+nofp" } */
> +
> +#include <stdarg.h>
> +
> +typedef int int32x2_t __attribute__ ((__vector_size__ ((8))));
> +
> +int test (int i, ...);
> +
> +int
> +main (int argc, char **argv)
> +{
> + int32x2_t a = (int32x2_t) {0, 1};
> + int32x2_t b = (int32x2_t) {2, 3};
> + return test (2, a, b); /* { dg-error "'\\+nofp' feature modifier is
> incompatible with vector argument" } */
> +}
<---->
> commit 298595e5254183de5b4c1cd2acaed43949b4dd30
> Author: Alan Lawrence <alan.lawre...@arm.com>
> Date: Mon Jan 19 12:18:02 2015 +0000
>
> doc/invoke.texi, as approved, with whitespace.
>
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index e25bd62..0d62edf 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -12359,7 +12359,10 @@ Generate big-endian code. This is the default when
> GCC is configured for an
>
> @item -mgeneral-regs-only
> @opindex mgeneral-regs-only
> -Generate code which uses only the general registers.
> +Generate code which uses only the general registers. Equivalent to feature
The ARMARM uses "general-purpose registers" to refer to these registers,
we should match that style.
s/Equivalent to feature/This is equivalent to the feature/
> +modifier @option{nofp} of @option{-march} or @option{-mcpu}, except that
> +@option{-mgeneral-regs-only} takes precedence over any conflicting feature
> +modifier regardless of sequence.
>
> @item -mlittle-endian
> @opindex mlittle-endian
> @@ -12498,22 +12501,28 @@ over the appropriate part of this option.
> @subsubsection @option{-march} and @option{-mcpu} Feature Modifiers
> @cindex @option{-march} feature modifiers
> @cindex @option{-mcpu} feature modifiers
> -Feature modifiers used with @option{-march} and @option{-mcpu} can be one
> -the following:
> +Feature modifiers used with @option{-march} and @option{-mcpu} can be any of
> +the following, or their inverses @option{no@var{feature}}:
s/inverses/inverse/
> @table @samp
> @item crc
> Enable CRC extension.
> @item crypto
> -Enable Crypto extension. This implies Advanced SIMD is enabled.
> +Enable Crypto extension. This also enables Advanced SIMD and floating-point
> +instructions.
> @item fp
> -Enable floating-point instructions.
> +Enable floating-point instructions. This is on by default for all possible
> +values for options @option{-march} and @option{-mcpu}.
> @item simd
> -Enable Advanced SIMD instructions. This implies floating-point instructions
> -are enabled. This is the default for all current possible values for options
> -@option{-march} and @option{-mcpu=}.
> +Enable Advanced SIMD instructions. This also enables floating-point
> +instructions. This is on by default for all possible values for options
> +@option{-march} and @option{-mcpu}.
> @end table
>
> +As stated above, @option{crypto} implies @option{simd} implies @option{fp}.
Drop the "As stated above".
> +Conversely, @option{nofp} (or equivalently, @option{-mgeneral-regs-only})
> +implies @option{nosimd} implies @option{nocrypto}.
> +
> @node Adapteva Epiphany Options
> @subsection Adapteva Epiphany Options
>
Thanks,
James
