[1/3,AArch64,nofp] Fix ICEs with +nofp/-mgeneral-regs-only and improve error messages

James Greenhalgh wrote:
> Submissions on this list should be one patch per mail, it makes
> tracking review easier.

OK here's a respin of the first, I've added a third patch after I found another 
route to get to an ICE.

>> +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).

Done.

>> +  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?

Dropped the args, added ATTRIBUTE_UNUSED to the others - the attribute isn't 
necessary at the moment but might become so if inlining became more aggressive.

This version has been bootstrapped on aarch64 linux.

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

Indeed - the new checks in init_cumulative_args and aarch64_layout_arg mean we 
never get here. (If said new checks were sorry(), we would still get here, but 
since they are error() we do not.)

>> @@ -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?

Similarly because of change from sorry() -> error().

>> 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?

(*2)

This usually gets called from emit_move_insn_1, via a call to emit_insn 
(GEN_FCN...). If we FAIL, we return NULL, and emit_insn then returns whatever 
insn was last in the BB; if we don't FAIL, we return a move to a general 
register (which is still a valid bit of RTL!). So either seems valid, but 
keeping the FAIL generates fewer instances of the error message, which can 
already get quite numerous. So reinstated FAIL. (Also changed "" quotes to {} 
braces.)

Bootstrap + check-gcc on aarch64-none-linux-gnu.

(ChangeLog's identical to v1)

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.

On Tue, Jun 23, 2015 at 05:02:46PM +0100, Alan Lawrence wrote:
> James Greenhalgh wrote:
 
> Bootstrap + check-gcc on aarch64-none-linux-gnu.
> 
> (ChangeLog's identical to v1)
> 
> 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.


OK.

Thanks,
James

> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index 965a11b7beeaaaa188819796e2b17017a87dca80..ac92c5924a4cfc5941fe8eeb31281e18bd21a5a0 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 a79bb6a96572799181a5bff3c3818e294f87cb7a..3193a15970e5524e0f3a8a5505baea5582e55731 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -522,6 +522,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";
> +  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)
>  {
> @@ -1772,6 +1782,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;
> @@ -1898,6 +1911,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 type = TREE_TYPE (fntype);
> +      machine_mode mode ATTRIBUTE_UNUSED; /* To pass pointer as argument.  */
> +      int nregs ATTRIBUTE_UNUSED; /* Likewise.  */
> +      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;
>  }
>  
> @@ -7557,9 +7581,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);
>        vr_save_area_size = 0;
>      }
>  
> @@ -7666,8 +7688,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);
> @@ -7904,9 +7925,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);
>        vr_saved = 0;
>      }
>  
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 1efe57c91b10e47ab7511d089f7b4bb53f18f06e..a9c41fdfee64784591a4a7360652d7da3e7f90d1 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -979,16 +979,16 @@
>    [(set (match_operand:GPF 0 "nonimmediate_operand" "")
>  	(match_operand:GPF 1 "general_operand" ""))]
>    ""
> -  "
> +  {
>      if (!TARGET_FLOAT)
> -     {
> -	sorry (\"%qs and floating point code\", \"-mgeneral-regs-only\");
> +      {
> +	aarch64_err_no_fpadvsimd (<MODE>mode, "code");
>  	FAIL;
> -     }
> +      }
>  
>      if (GET_CODE (operands[0]) == MEM)
>        operands[1] = force_reg (<MODE>mode, operands[1]);
> -  "
> +  }
>  )
>  
>  (define_insn "*movsf_aarch64"
> @@ -1033,18 +1033,18 @@
>    [(set (match_operand:TF 0 "nonimmediate_operand" "")
>  	(match_operand:TF 1 "general_operand" ""))]
>    ""
> -  "
> +  {
>      if (!TARGET_FLOAT)
> -     {
> -	sorry (\"%qs and floating point code\", \"-mgeneral-regs-only\");
> +      {
> +	aarch64_err_no_fpadvsimd (TFmode, "code");
>  	FAIL;
> -     }
> +      }
>  
>      if (GET_CODE (operands[0]) == MEM
>          && ! (GET_CODE (operands[1]) == CONST_DOUBLE
>  	      && aarch64_float_const_zero_rtx_p (operands[1])))
>        operands[1] = force_reg (TFmode, operands[1]);
> -  "
> +  }
>  )
>  
>  (define_insn "*movtf_aarch64"
> 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 0000000000000000000000000000000000000000..b5192a6a4838c14fcf0d00cd4d33b7c7255abd80
> --- /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 0000000000000000000000000000000000000000..859019970ae044d8b36a095329c9e47a2826a399
> --- /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 0000000000000000000000000000000000000000..3fc00368668e2d9cfbf3b6c83d72cb9ebbb84821
> --- /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" } */
> +}