[PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store

[PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store

[charles.baylis]

From: Charles Baylis <charles.baylis@linaro.org>

This patch series moves the checking of lane indices for vld[234](q?)_lane and
vst[234](q?)_lane intrinsics so that it occurs during builtin expansion.

The q register variants are checked directly, but since the d register variants
use the same intrinsics, these are checked in arm_neon.h using
__builtin_aarch64_im_land_boundsi().

Tested with make check-gcc on aarch64-oe-linux, with no regressions.

Charles Baylis (4):
  vldN_lane error message enhancements (Q registers)
  vldN_lane error message enhancements (D registers)
  vstN_lane error message enhancements (Q register)
  vstN_lane error message enhancements (D registers)

 gcc/config/aarch64/aarch64-builtins.c | 32 +++++++++++++++++++++++++++-----
 gcc/config/aarch64/aarch64-simd.md    | 12 ++++++------
 gcc/config/aarch64/arm_neon.h         | 12 ++++++++++++
 3 files changed, 45 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH 3/4] vstN_lane error message enhancements (Q register)

[charles.baylis]

From: Charles Baylis <charles.baylis@linaro.org>

gcc/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * config/aarch64/aarch64-builtins.c
	(aarch64_types_storestruct_lane_qualifiers): Mark last argument with
	qualifier_struct_load_store_lane_index.

gcc/testsuite/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * gcc.target/aarch64/simd/vst4q_lane.c: New test.

Change-Id: If097c9d32eb6eb3d4c4e16db81f81e44a3154509
---
 gcc/config/aarch64/aarch64-builtins.c              |  2 +-
 gcc/testsuite/gcc.target/aarch64/simd/vst4q_lane.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/vst4q_lane.c

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 27046e2..f2fb939 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -258,7 +258,7 @@ aarch64_types_store1_qualifiers[SIMD_MAX_BUILTIN_ARGS]
 static enum aarch64_type_qualifiers
 aarch64_types_storestruct_lane_qualifiers[SIMD_MAX_BUILTIN_ARGS]
   = { qualifier_void, qualifier_pointer_map_mode,
-      qualifier_none, qualifier_none };
+      qualifier_none, qualifier_struct_load_store_lane_index };
 #define TYPES_STORESTRUCT_LANE (aarch64_types_storestruct_lane_qualifiers)
 
 #define CF0(N, X) CODE_FOR_aarch64_##N##X
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vst4q_lane.c b/gcc/testsuite/gcc.target/aarch64/simd/vst4q_lane.c
new file mode 100644
index 0000000..849f07a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vst4q_lane.c
@@ -0,0 +1,15 @@
+/* Test error message when passing an invalid value as a lane index.  */
+
+/* { dg-do compile } */
+
+#include <arm_neon.h>
+
+void
+f_vst4q_lane (int8_t * p, int8x16x4_t v)
+{
+  vst4q_lane_s8 (p, v, 16);
+/* { dg-error "lane 16 out of range 0 - 15" "" { target *-*-* } 0 } */
+  return;
+}
+
+
-- 
1.9.1

[PATCH 1/4] vldN_lane error message enhancements (Q registers)

[charles.baylis]

From: Charles Baylis <charles.baylis@linaro.org>

gcc/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

	PR target/63870
        * config/aarch64/aarch64-builtins.c (enum aarch64_type_qualifiers):
	Add qualifier_struct_load_store_lane_index.
	(aarch64_types_loadstruct_lane_qualifiers): Use
	qualifier_struct_load_store_lane_index for lane index argument for
	last argument.
	(builtin_simd_arg): Add SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX.
        (aarch64_simd_expand_args): Add new argument describing mode of
	builtin. Check lane bounds for arguments with
	SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX.
        (aarch64_simd_expand_builtin): Emit error for incorrect lane indices
	if marked with SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX.
	(aarch64_simd_expand_builtin): Pass machine mode of builtin to
	aarch64_simd_expand_args.
        * config/aarch64/aarch64-simd.md: (aarch64_ld2_lane<mode>): Remove
	lane bounds check. Adjust lane numbers for big-endian.
	(aarch64_ld3_lane<mode>): Likewise.
	(aarch64_ld4_lane<mode>): Likewise.

gcc/testsuite/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * gcc.target/aarch64/simd/vld4q_lane.c: New test.

Change-Id: Ib17adaf64e631cf8d00a1a1a6c12409d2d7f4239
---
 gcc/config/aarch64/aarch64-builtins.c              | 30 +++++++++++++++++++---
 gcc/config/aarch64/aarch64-simd.md                 | 12 ++++-----
 gcc/testsuite/gcc.target/aarch64/simd/vld4q_lane.c | 16 ++++++++++++
 3 files changed, 48 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/vld4q_lane.c

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index aac7269..27046e2 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -116,7 +116,9 @@ enum aarch64_type_qualifiers
   /* Polynomial types.  */
   qualifier_poly = 0x100,
   /* Lane indices - must be in range, and flipped for bigendian.  */
-  qualifier_lane_index = 0x200
+  qualifier_lane_index = 0x200,
+  /* Lane indices for single lane structure loads and stores */
+  qualifier_struct_load_store_lane_index = 0x400
 };
 
 typedef struct
@@ -224,7 +226,7 @@ aarch64_types_load1_qualifiers[SIMD_MAX_BUILTIN_ARGS]
 static enum aarch64_type_qualifiers
 aarch64_types_loadstruct_lane_qualifiers[SIMD_MAX_BUILTIN_ARGS]
   = { qualifier_none, qualifier_const_pointer_map_mode,
-      qualifier_none, qualifier_none };
+      qualifier_none, qualifier_struct_load_store_lane_index };
 #define TYPES_LOADSTRUCT_LANE (aarch64_types_loadstruct_lane_qualifiers)
 
 static enum aarch64_type_qualifiers
@@ -859,12 +861,14 @@ typedef enum
   SIMD_ARG_COPY_TO_REG,
   SIMD_ARG_CONSTANT,
   SIMD_ARG_LANE_INDEX,
+  SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX,
   SIMD_ARG_STOP
 } builtin_simd_arg;
 
 static rtx
 aarch64_simd_expand_args (rtx target, int icode, int have_retval,
-			  tree exp, builtin_simd_arg *args)
+			  tree exp, builtin_simd_arg *args,
+			  enum machine_mode builtin_mode)
 {
   rtx pat;
   rtx op[SIMD_MAX_BUILTIN_ARGS + 1]; /* First element for result operand.  */
@@ -903,6 +907,21 @@ aarch64_simd_expand_args (rtx target, int icode, int have_retval,
 		op[opc] = copy_to_mode_reg (mode, op[opc]);
 	      break;
 
+	    case SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX:
+	      /* we expect arguments in order (ptr, array_of_vector, lane), and
+		 we have to grub around in the ptr to find the lane size */
+	      gcc_assert (opc > 1);
+	      if (CONST_INT_P (op[opc]))
+		{
+		  aarch64_simd_lane_bounds (op[opc], 0,
+					    GET_MODE_NUNITS (builtin_mode),
+					    exp);
+		  /* Keep to GCC-vector-extension lane indices in the RTL.  */
+		  op[opc] =
+		    GEN_INT (ENDIAN_LANE_N (builtin_mode, INTVAL (op[opc])));
+		}
+	      goto constant_arg;
+
 	    case SIMD_ARG_LANE_INDEX:
 	      /* Must be a previous operand into which this is an index.  */
 	      gcc_assert (opc > 0);
@@ -917,6 +936,7 @@ aarch64_simd_expand_args (rtx target, int icode, int have_retval,
 	      /* Fall through - if the lane index isn't a constant then
 		 the next case will error.  */
 	    case SIMD_ARG_CONSTANT:
+constant_arg:
 	      if (!(*insn_data[icode].operand[opc].predicate)
 		  (op[opc], mode))
 	      {
@@ -1003,6 +1023,8 @@ aarch64_simd_expand_builtin (int fcode, tree exp, rtx target)
 
       if (d->qualifiers[qualifiers_k] & qualifier_lane_index)
 	args[k] = SIMD_ARG_LANE_INDEX;
+      else if (d->qualifiers[qualifiers_k] & qualifier_struct_load_store_lane_index)
+	args[k] = SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX;
       else if (d->qualifiers[qualifiers_k] & qualifier_immediate)
 	args[k] = SIMD_ARG_CONSTANT;
       else if (d->qualifiers[qualifiers_k] & qualifier_maybe_immediate)
@@ -1026,7 +1048,7 @@ aarch64_simd_expand_builtin (int fcode, tree exp, rtx target)
   /* The interface to aarch64_simd_expand_args expects a 0 if
      the function is void, and a 1 if it is not.  */
   return aarch64_simd_expand_args
-	  (target, icode, !is_void, exp, &args[1]);
+	  (target, icode, !is_void, exp, &args[1], d->mode);
 }
 
 rtx
diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index 0ec1323..beac497 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -4397,8 +4397,8 @@
   machine_mode mode = <V_TWO_ELEM>mode;
   rtx mem = gen_rtx_MEM (mode, operands[1]);
 
-  aarch64_simd_lane_bounds (operands[3], 0, GET_MODE_NUNITS (<VCONQ>mode),
-			    NULL);
+  operands[3] = GEN_INT (ENDIAN_LANE_N (<MODE>mode, INTVAL (operands[3])));
+
   emit_insn (gen_aarch64_vec_load_lanesoi_lane<mode> (operands[0],
 						      mem,
 						      operands[2],
@@ -4417,8 +4417,8 @@
   machine_mode mode = <V_THREE_ELEM>mode;
   rtx mem = gen_rtx_MEM (mode, operands[1]);
 
-  aarch64_simd_lane_bounds (operands[3], 0, GET_MODE_NUNITS (<VCONQ>mode),
-			    NULL);
+  operands[3] = GEN_INT (ENDIAN_LANE_N (<MODE>mode, INTVAL (operands[3])));
+
   emit_insn (gen_aarch64_vec_load_lanesci_lane<mode> (operands[0],
 						      mem,
 						      operands[2],
@@ -4437,8 +4437,8 @@
   machine_mode mode = <V_FOUR_ELEM>mode;
   rtx mem = gen_rtx_MEM (mode, operands[1]);
 
-  aarch64_simd_lane_bounds (operands[3], 0, GET_MODE_NUNITS (<VCONQ>mode),
-			    NULL);
+  operands[3] = GEN_INT (ENDIAN_LANE_N (<MODE>mode, INTVAL (operands[3])));
+
   emit_insn (gen_aarch64_vec_load_lanesxi_lane<mode> (operands[0],
 						      mem,
 						      operands[2],
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vld4q_lane.c b/gcc/testsuite/gcc.target/aarch64/simd/vld4q_lane.c
new file mode 100644
index 0000000..c673ded
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vld4q_lane.c
@@ -0,0 +1,16 @@
+/* Test error message when passing an invalid value as a lane index.  */
+
+/* { dg-do compile } */
+
+#include <arm_neon.h>
+
+/* { dg-error "lane 8 out of range 0 - 7" "" { target *-*-* } 0 } */
+int16x8x4_t
+f_vld4_lane (int16_t * p, int16x8x4_t v)
+{
+  int16x8x4_t res;
+  res = vld4q_lane_s16 (p, v, 8);
+  return res;
+}
+
+
-- 
1.9.1

Re: [PATCH 1/4] vldN_lane error message enhancements (Q registers)

[Charles Baylis]

On 14 April 2015 at 14:45, Alan Lawrence <alan.lawrence@arm.com> wrote:

> Assuming/hoping that this patch is proposed for new stage 1 ;),

IIRC the approach of using __builtin_aarch64_im_lane_boundsi doesn't
work (results in double error messages), and so the patch needs to be
rewritten to avoid it. However, thanks for your comments, I'll reflect
those in the next version of the patch.

Thanks
Charles

Re: [PATCH 1/4] vldN_lane error message enhancements (Q registers)

[Alan Lawrence]

That happens in your patch 2/3/4, which use __builtin_aarch64_im_lane_boundsi, 
indeed. Hence I think the SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX approach of the 
first patch could well be the right way - initially I thought SIMD_ARG... too 
heavyweight, but I think I take that back now.

Really I think we should clean up and stop using q-reg intrinsics to handle 
d-regs here. I'm working on a few patches (i.e. targetting the v{st,ld}{2,3,4}* 
intrinsics) with that aim now, I think I can make some efficiency improvements 
in the process, too....

--Alan

Charles Baylis wrote:
> On 14 April 2015 at 14:45, Alan Lawrence <alan.lawrence@arm.com> wrote:
> 
>> Assuming/hoping that this patch is proposed for new stage 1 ;),
> 
> IIRC the approach of using __builtin_aarch64_im_lane_boundsi doesn't
> work (results in double error messages), and so the patch needs to be
> rewritten to avoid it. However, thanks for your comments, I'll reflect
> those in the next version of the patch.
> 
> Thanks
> Charles
> 

[PATCH 2/4] vldN_lane error message enhancements (D registers)

[charles.baylis]

From: Charles Baylis <charles.baylis@linaro.org>

gcc/ChangeLog

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * config/aarch64/arm_neon.h (__LD2_LANE_FUNC): Add explicit lane
	bounds check.
        (__LD3_LANE_FUNC): Likewise.
        (__LD4_LANE_FUNC): Likewise

gcc/testsuite/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * gcc.target/aarch64/simd/vld4_lane.c: New test.

Change-Id: Ia95fbed34b50cf710ea9032ff3428a5f1432e0aa
---
 gcc/config/aarch64/arm_neon.h                     |  6 ++++++
 gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c | 15 +++++++++++++++
 2 files changed, 21 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c

diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h
index 8cff719..22df564 100644
--- a/gcc/config/aarch64/arm_neon.h
+++ b/gcc/config/aarch64/arm_neon.h
@@ -17901,6 +17901,8 @@ vld2_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
   __o = __builtin_aarch64_set_qregoi##mode (__o,			   \
 					   (signedtype) __temp.val[1],	   \
 					   1);				   \
+  __builtin_aarch64_im_lane_boundsi (__c,				   \
+				     sizeof (vectype) / sizeof (*__ptr));  \
   __o =	__builtin_aarch64_ld2_lane##mode (				   \
 	  (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);	   \
   __b.val[0] = (vectype) __builtin_aarch64_get_dregoidi (__o, 0);	   \
@@ -17991,6 +17993,8 @@ vld3_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
   __o = __builtin_aarch64_set_qregci##mode (__o,			   \
 					   (signedtype) __temp.val[2],	   \
 					   2);				   \
+  __builtin_aarch64_im_lane_boundsi (__c,				   \
+				     sizeof (vectype) / sizeof (*__ptr));  \
   __o =	__builtin_aarch64_ld3_lane##mode (				   \
 	  (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);	   \
   __b.val[0] = (vectype) __builtin_aarch64_get_dregcidi (__o, 0);	   \
@@ -18089,6 +18093,8 @@ vld4_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
   __o = __builtin_aarch64_set_qregxi##mode (__o,			   \
 					   (signedtype) __temp.val[3],	   \
 					   3);				   \
+  __builtin_aarch64_im_lane_boundsi (__c,				   \
+				     sizeof (vectype) / sizeof (*__ptr));  \
   __o =	__builtin_aarch64_ld4_lane##mode (				   \
 	  (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);	   \
   __b.val[0] = (vectype) __builtin_aarch64_get_dregxidi (__o, 0);	   \
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c b/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
new file mode 100644
index 0000000..d14e6c1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
@@ -0,0 +1,15 @@
+/* Test error message when passing an invalid value as a lane index.  */
+
+/* { dg-do compile } */
+
+#include <arm_neon.h>
+
+int8x8x4_t
+f_vld4_lane (int8_t * p, int8x8x4_t v)
+{
+  int8x8x4_t res;
+  /* { dg-error "lane 8 out of range 0 - 7" "" { target *-*-* } 0 } */
+  res = vld4_lane_s8 (p, v, 8);
+  return res;
+}
+
-- 
1.9.1

Re: [PATCH 2/4] vldN_lane error message enhancements (D registers)

[Christophe Lyon]

On 9 December 2014 at 16:27,  <charles.baylis@linaro.org> wrote:
> From: Charles Baylis <charles.baylis@linaro.org>
>
> gcc/ChangeLog
>
> <DATE>  Charles Baylis  <charles.baylis@linaro.org>
>
>         * config/aarch64/arm_neon.h (__LD2_LANE_FUNC): Add explicit lane
>         bounds check.
>         (__LD3_LANE_FUNC): Likewise.
>         (__LD4_LANE_FUNC): Likewise
>
> gcc/testsuite/ChangeLog:
>
> <DATE>  Charles Baylis  <charles.baylis@linaro.org>
>
>         * gcc.target/aarch64/simd/vld4_lane.c: New test.
>
> Change-Id: Ia95fbed34b50cf710ea9032ff3428a5f1432e0aa
> ---
>  gcc/config/aarch64/arm_neon.h                     |  6 ++++++
>  gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c | 15 +++++++++++++++
>  2 files changed, 21 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
>
> diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h
> index 8cff719..22df564 100644
> --- a/gcc/config/aarch64/arm_neon.h
> +++ b/gcc/config/aarch64/arm_neon.h
> @@ -17901,6 +17901,8 @@ vld2_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
>    __o = __builtin_aarch64_set_qregoi##mode (__o,                          \
>                                            (signedtype) __temp.val[1],     \
>                                            1);                             \
> +  __builtin_aarch64_im_lane_boundsi (__c,                                 \
> +                                    sizeof (vectype) / sizeof (*__ptr));  \

Shouldn't the arguments be reversed? (I'm looking at
__AARCH64_LANE_CHECK: the lane index is the 2nd parameter)

>    __o =        __builtin_aarch64_ld2_lane##mode (                                 \
>           (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);           \
>    __b.val[0] = (vectype) __builtin_aarch64_get_dregoidi (__o, 0);         \
> @@ -17991,6 +17993,8 @@ vld3_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
>    __o = __builtin_aarch64_set_qregci##mode (__o,                          \
>                                            (signedtype) __temp.val[2],     \
>                                            2);                             \
> +  __builtin_aarch64_im_lane_boundsi (__c,                                 \
> +                                    sizeof (vectype) / sizeof (*__ptr));  \
>    __o =        __builtin_aarch64_ld3_lane##mode (                                 \
>           (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);           \
>    __b.val[0] = (vectype) __builtin_aarch64_get_dregcidi (__o, 0);         \
> @@ -18089,6 +18093,8 @@ vld4_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
>    __o = __builtin_aarch64_set_qregxi##mode (__o,                          \
>                                            (signedtype) __temp.val[3],     \
>                                            3);                             \
> +  __builtin_aarch64_im_lane_boundsi (__c,                                 \
> +                                    sizeof (vectype) / sizeof (*__ptr));  \
>    __o =        __builtin_aarch64_ld4_lane##mode (                                 \
>           (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);           \
>    __b.val[0] = (vectype) __builtin_aarch64_get_dregxidi (__o, 0);         \
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c b/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
> new file mode 100644
> index 0000000..d14e6c1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
> @@ -0,0 +1,15 @@
> +/* Test error message when passing an invalid value as a lane index.  */
> +
> +/* { dg-do compile } */
> +
> +#include <arm_neon.h>
> +
> +int8x8x4_t
> +f_vld4_lane (int8_t * p, int8x8x4_t v)
> +{
> +  int8x8x4_t res;
> +  /* { dg-error "lane 8 out of range 0 - 7" "" { target *-*-* } 0 } */
> +  res = vld4_lane_s8 (p, v, 8);
> +  return res;
> +}
> +
> --
> 1.9.1
>

Re: [PATCH 2/4] vldN_lane error message enhancements (D registers)

[Alan Lawrence]

Hmmm. Yes I think I may have switched that in the patch introducing 
__AARCH64_LANE_CHECK, and it was correct at time of Charles' writing. However, 
maybe we could (now) use __AARCH64_LANE_CHECK directly? (Referencing one of the 
component vectors in the blahLxMxN_t struct?)

--Alan

Christophe Lyon wrote:
> On 9 December 2014 at 16:27,  <charles.baylis@linaro.org> wrote:
>> From: Charles Baylis <charles.baylis@linaro.org>
>>
>> gcc/ChangeLog
>>
>> <DATE>  Charles Baylis  <charles.baylis@linaro.org>
>>
>>         * config/aarch64/arm_neon.h (__LD2_LANE_FUNC): Add explicit lane
>>         bounds check.
>>         (__LD3_LANE_FUNC): Likewise.
>>         (__LD4_LANE_FUNC): Likewise
>>
>> gcc/testsuite/ChangeLog:
>>
>> <DATE>  Charles Baylis  <charles.baylis@linaro.org>
>>
>>         * gcc.target/aarch64/simd/vld4_lane.c: New test.
>>
>> Change-Id: Ia95fbed34b50cf710ea9032ff3428a5f1432e0aa
>> ---
>>  gcc/config/aarch64/arm_neon.h                     |  6 ++++++
>>  gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c | 15 +++++++++++++++
>>  2 files changed, 21 insertions(+)
>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
>>
>> diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h
>> index 8cff719..22df564 100644
>> --- a/gcc/config/aarch64/arm_neon.h
>> +++ b/gcc/config/aarch64/arm_neon.h
>> @@ -17901,6 +17901,8 @@ vld2_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
>>    __o = __builtin_aarch64_set_qregoi##mode (__o,                          \
>>                                            (signedtype) __temp.val[1],     \
>>                                            1);                             \
>> +  __builtin_aarch64_im_lane_boundsi (__c,                                 \
>> +                                    sizeof (vectype) / sizeof (*__ptr));  \
> 
> Shouldn't the arguments be reversed? (I'm looking at
> __AARCH64_LANE_CHECK: the lane index is the 2nd parameter)
> 
>>    __o =        __builtin_aarch64_ld2_lane##mode (                                 \
>>           (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);           \
>>    __b.val[0] = (vectype) __builtin_aarch64_get_dregoidi (__o, 0);         \
>> @@ -17991,6 +17993,8 @@ vld3_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
>>    __o = __builtin_aarch64_set_qregci##mode (__o,                          \
>>                                            (signedtype) __temp.val[2],     \
>>                                            2);                             \
>> +  __builtin_aarch64_im_lane_boundsi (__c,                                 \
>> +                                    sizeof (vectype) / sizeof (*__ptr));  \
>>    __o =        __builtin_aarch64_ld3_lane##mode (                                 \
>>           (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);           \
>>    __b.val[0] = (vectype) __builtin_aarch64_get_dregcidi (__o, 0);         \
>> @@ -18089,6 +18093,8 @@ vld4_lane_##funcsuffix (const ptrtype * __ptr, intype __b, const int __c)  \
>>    __o = __builtin_aarch64_set_qregxi##mode (__o,                          \
>>                                            (signedtype) __temp.val[3],     \
>>                                            3);                             \
>> +  __builtin_aarch64_im_lane_boundsi (__c,                                 \
>> +                                    sizeof (vectype) / sizeof (*__ptr));  \
>>    __o =        __builtin_aarch64_ld4_lane##mode (                                 \
>>           (__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c);           \
>>    __b.val[0] = (vectype) __builtin_aarch64_get_dregxidi (__o, 0);         \
>> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c b/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
>> new file mode 100644
>> index 0000000..d14e6c1
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c
>> @@ -0,0 +1,15 @@
>> +/* Test error message when passing an invalid value as a lane index.  */
>> +
>> +/* { dg-do compile } */
>> +
>> +#include <arm_neon.h>
>> +
>> +int8x8x4_t
>> +f_vld4_lane (int8_t * p, int8x8x4_t v)
>> +{
>> +  int8x8x4_t res;
>> +  /* { dg-error "lane 8 out of range 0 - 7" "" { target *-*-* } 0 } */
>> +  res = vld4_lane_s8 (p, v, 8);
>> +  return res;
>> +}
>> +
>> --
>> 1.9.1
>>
> 
	

[PATCH 4/4] vstN_lane error message enhancements (D registers)

[charles.baylis]

From: Charles Baylis <charles.baylis@linaro.org>

gcc/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * config/aarch64/arm_neon.h (__ST2_LANE_FUNC): Add explicit lane bounds
	check.
        (__ST3_LANE_FUNC): Likewise.
        (__ST4_LANE_FUNC): Likewise.

gcc/testsuite/ChangeLog:

<DATE>  Charles Baylis  <charles.baylis@linaro.org>

        * gcc.target/aarch64/simd/vst4_lane.c: New test.

Change-Id: I6bceaeb7773bf20860daca4013ea1c4d2c06afa6
---
 gcc/config/aarch64/arm_neon.h                     |  6 ++++++
 gcc/testsuite/gcc.target/aarch64/simd/vst4_lane.c | 14 ++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/vst4_lane.c

diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h
index 22df564..22c6d06 100644
--- a/gcc/config/aarch64/arm_neon.h
+++ b/gcc/config/aarch64/arm_neon.h
@@ -11181,6 +11181,8 @@ vst2_lane_ ## funcsuffix (ptrtype *__ptr,				     \
   __temp.val[1]								     \
     = vcombine_##funcsuffix (__b.val[1],				     \
 			     vcreate_##funcsuffix (__AARCH64_UINT64_C (0))); \
+  __builtin_aarch64_im_lane_boundsi (__c,				     \
+				     sizeof (__b.val[0]) / sizeof (*__ptr)); \
   __o = __builtin_aarch64_set_qregoi##mode (__o,			     \
 					    (signedtype) __temp.val[0], 0);  \
   __o = __builtin_aarch64_set_qregoi##mode (__o,			     \
@@ -11258,6 +11260,8 @@ vst3_lane_ ## funcsuffix (ptrtype *__ptr,				     \
 					    (signedtype) __temp.val[1], 1);  \
   __o = __builtin_aarch64_set_qregci##mode (__o,			     \
 					    (signedtype) __temp.val[2], 2);  \
+  __builtin_aarch64_im_lane_boundsi (__c,				     \
+				     sizeof (__b.val[0]) / sizeof (*__ptr)); \
   __builtin_aarch64_st3_lane##mode ((__builtin_aarch64_simd_ ## ptr_mode *)  \
 				     __ptr, __o, __c);			     \
 }
@@ -11336,6 +11340,8 @@ vst4_lane_ ## funcsuffix (ptrtype *__ptr,				     \
 					    (signedtype) __temp.val[2], 2);  \
   __o = __builtin_aarch64_set_qregxi##mode (__o,			     \
 					    (signedtype) __temp.val[3], 3);  \
+  __builtin_aarch64_im_lane_boundsi (__c,				     \
+				     sizeof (__b.val[0]) / sizeof (*__ptr)); \
   __builtin_aarch64_st4_lane##mode ((__builtin_aarch64_simd_ ## ptr_mode *)  \
 				     __ptr, __o, __c);			     \
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vst4_lane.c b/gcc/testsuite/gcc.target/aarch64/simd/vst4_lane.c
new file mode 100644
index 0000000..6627ecf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vst4_lane.c
@@ -0,0 +1,14 @@
+/* Test error message when passing an invalid value as a lane index.  */
+
+/* { dg-do compile } */
+
+#include <arm_neon.h>
+
+void
+f_vst4_lane (int8_t * p, int8x8x4_t v)
+{
+  /* { dg-error "lane 8 out of range 0 - 7" "" { target *-*-* } 0 } */
+  vst4_lane_s8 (p, v, 8);
+  return;
+}
+
-- 
1.9.1

Re: [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store

[Alan Lawrence]

Thanks, Charles. A couple of thoughts.

I think the approach in patches 2+3+4 of using __builtin_aarch64_im_lane_boundsi 
is justified and works quite neatly. Modulo the question of argument ordering 
and __AARCH64_LANE_CHECK, those patches look good.

However, the SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX, seems a lot of 
infrastructure to introduce if we are only going to use it in one place, and I 
think I might argue in favour of using ...__im_lane_bound or AARCH64_LANE_CHECK 
there also. Of course all of this palaver stems from using the same builtins for 
both D- and Q-reg intrinsics, and I suspect some cleanup may be due to those 
intrinsics *at some point*, but probably not in time for gcc 5.0. However, this 
does mean that if I use a D-reg intrinsic with a lane index that's out of bounds 
for the Q-reg too, I get a double error message: e.g. for testcase

int8x8x4_t
f_vld4_lane (int8_t * p, int8x8x4_t v)
{
   int8x8x4_t res;
   return vld4_lane_s8 (p, v, 18);
}

I get output:

In file included from gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c:5:0:
.../install/lib/gcc/aarch64-none-elf/5.0.0/include/arm_neon.h: In function 
'f_vld4_lane':
.../install/lib/gcc/aarch64-none-elf/5.0.0/include/arm_neon.h:18123:1: error: 
lane 18 out of range 0 - 7
  __LD4_LANE_FUNC (int8x8x4_t, int8x8_t, int8x16x4_t, int8_t, v16qi, qi, s8,
  ^
In function 'vld4_lane_s8',
     inlined from 'f_vld4_lane' at 
gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c:12:7:
.../install/lib/gcc/aarch64-none-elf/5.0.0/include/arm_neon.h:18123:1: error: 
lane 18 out of range 0 - 15
  __LD4_LANE_FUNC (int8x8x4_t, int8x8_t, int8x16x4_t, int8_t, v16qi, qi, s8,
  ^

which (although not serious) could be mildly confusing.

--Alan

charles.baylis@linaro.org wrote:
> From: Charles Baylis <charles.baylis@linaro.org>
> 
> This patch series moves the checking of lane indices for vld[234](q?)_lane and
> vst[234](q?)_lane intrinsics so that it occurs during builtin expansion.
> 
> The q register variants are checked directly, but since the d register variants
> use the same intrinsics, these are checked in arm_neon.h using
> __builtin_aarch64_im_land_boundsi().
> 
> Tested with make check-gcc on aarch64-oe-linux, with no regressions.
> 
> Charles Baylis (4):
>   vldN_lane error message enhancements (Q registers)
>   vldN_lane error message enhancements (D registers)
>   vstN_lane error message enhancements (Q register)
>   vstN_lane error message enhancements (D registers)
> 
>  gcc/config/aarch64/aarch64-builtins.c | 32 +++++++++++++++++++++++++++-----
>  gcc/config/aarch64/aarch64-simd.md    | 12 ++++++------
>  gcc/config/aarch64/arm_neon.h         | 12 ++++++++++++
>  3 files changed, 45 insertions(+), 11 deletions(-)
> 

Re: [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store

[Charles Baylis]

On 10 December 2014 at 10:34, Alan Lawrence <alan.lawrence@arm.com> wrote:
> Thanks, Charles. A couple of thoughts.
>
> I think the approach in patches 2+3+4 of using
> __builtin_aarch64_im_lane_boundsi is justified and works quite neatly.
> Modulo the question of argument ordering and __AARCH64_LANE_CHECK, those
> patches look good.
>
> However, the SIMD_ARG_STRUCT_LOAD_STORE_LANE_INDEX, seems a lot of
> infrastructure to introduce if we are only going to use it in one place, and
> I think I might argue in favour of using ...__im_lane_bound or
> AARCH64_LANE_CHECK there also. Of course all of this palaver stems from
> using the same builtins for both D- and Q-reg intrinsics, and I suspect some
> cleanup may be due to those intrinsics *at some point*, but probably not in
> time for gcc 5.0.
>
> However, this does mean that if I use a D-reg intrinsic
> with a lane index that's out of bounds for the Q-reg too, I get a double
> error message: e.g. for testcase
>
> int8x8x4_t
> f_vld4_lane (int8_t * p, int8x8x4_t v)
> {
>   int8x8x4_t res;
>   return vld4_lane_s8 (p, v, 18);
> }
>
> I get output:
>
> In file included from gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c:5:0:
> .../install/lib/gcc/aarch64-none-elf/5.0.0/include/arm_neon.h: In function
> 'f_vld4_lane':
> .../install/lib/gcc/aarch64-none-elf/5.0.0/include/arm_neon.h:18123:1:
> error: lane 18 out of range 0 - 7
>  __LD4_LANE_FUNC (int8x8x4_t, int8x8_t, int8x16x4_t, int8_t, v16qi, qi, s8,
>  ^
> In function 'vld4_lane_s8',
>     inlined from 'f_vld4_lane' at
> gcc/testsuite/gcc.target/aarch64/simd/vld4_lane.c:12:7:
> .../install/lib/gcc/aarch64-none-elf/5.0.0/include/arm_neon.h:18123:1:
> error: lane 18 out of range 0 - 15
>  __LD4_LANE_FUNC (int8x8x4_t, int8x8_t, int8x16x4_t, int8_t, v16qi, qi, s8,
>  ^
>
> which (although not serious) could be mildly confusing.

Oh dear, this is rather sad. Aesthetically, I think the builtins
should protect themselves from direct misuse, but I can't think of a
clean way to prevent this.

It could be done like this, but I don't think the end result really
justifies it.

  __o = __builtin_aarch64_ld4_lane##mode
((__builtin_aarch64_simd_##ptrmode *) __ptr, __o, __c &
(__NUMBER_OF_LANES(__b.val[0]) - 1));