* [PATCH 2/4] vldN_lane error message enhancements (D registers)
2014-12-09 15:28 [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store charles.baylis
2014-12-09 15:28 ` [PATCH 1/4] vldN_lane error message enhancements (Q registers) charles.baylis
@ 2014-12-09 15:28 ` charles.baylis
2014-12-10 9:23 ` Christophe Lyon
2014-12-09 15:28 ` [PATCH 4/4] vstN_lane " charles.baylis
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: charles.baylis @ 2014-12-09 15:28 UTC (permalink / raw)
To: rearnsha, gcc-patches, marcus.shawcroft, tejas.belagod, alan.lawrence
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] vstN_lane error message enhancements (Q register)
2014-12-09 15:28 [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store charles.baylis
` (2 preceding siblings ...)
2014-12-09 15:28 ` [PATCH 4/4] vstN_lane " charles.baylis
@ 2014-12-09 15:28 ` charles.baylis
2014-12-10 10:34 ` [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store Alan Lawrence
4 siblings, 0 replies; 11+ messages in thread
From: charles.baylis @ 2014-12-09 15:28 UTC (permalink / raw)
To: rearnsha, gcc-patches, marcus.shawcroft, tejas.belagod, alan.lawrence
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] vldN_lane error message enhancements (Q registers)
2014-12-09 15:28 [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store charles.baylis
@ 2014-12-09 15:28 ` charles.baylis
[not found] ` <CAOckXuMSZyHU80A6-VjSzZ7bHrtRikTKwn-45DZ9oPPuRX_0HQ@mail.gmail.com>
2014-12-09 15:28 ` [PATCH 2/4] vldN_lane error message enhancements (D registers) charles.baylis
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: charles.baylis @ 2014-12-09 15:28 UTC (permalink / raw)
To: rearnsha, gcc-patches, marcus.shawcroft, tejas.belagod, alan.lawrence
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] vstN_lane error message enhancements (D registers)
2014-12-09 15:28 [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store charles.baylis
2014-12-09 15:28 ` [PATCH 1/4] vldN_lane error message enhancements (Q registers) charles.baylis
2014-12-09 15:28 ` [PATCH 2/4] vldN_lane error message enhancements (D registers) charles.baylis
@ 2014-12-09 15:28 ` charles.baylis
2014-12-09 15:28 ` [PATCH 3/4] vstN_lane error message enhancements (Q register) charles.baylis
2014-12-10 10:34 ` [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store Alan Lawrence
4 siblings, 0 replies; 11+ messages in thread
From: charles.baylis @ 2014-12-09 15:28 UTC (permalink / raw)
To: rearnsha, gcc-patches, marcus.shawcroft, tejas.belagod, alan.lawrence
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store
@ 2014-12-09 15:28 charles.baylis
2014-12-09 15:28 ` [PATCH 1/4] vldN_lane error message enhancements (Q registers) charles.baylis
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: charles.baylis @ 2014-12-09 15:28 UTC (permalink / raw)
To: rearnsha, gcc-patches, marcus.shawcroft, tejas.belagod, alan.lawrence
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] vldN_lane error message enhancements (D registers)
2014-12-09 15:28 ` [PATCH 2/4] vldN_lane error message enhancements (D registers) charles.baylis
@ 2014-12-10 9:23 ` Christophe Lyon
2014-12-10 10:26 ` Alan Lawrence
0 siblings, 1 reply; 11+ messages in thread
From: Christophe Lyon @ 2014-12-10 9:23 UTC (permalink / raw)
To: Charles Baylis
Cc: Richard Earnshaw, gcc-patches, Marcus Shawcroft, Tejas Belagod,
Alan Lawrence
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] vldN_lane error message enhancements (D registers)
2014-12-10 9:23 ` Christophe Lyon
@ 2014-12-10 10:26 ` Alan Lawrence
0 siblings, 0 replies; 11+ messages in thread
From: Alan Lawrence @ 2014-12-10 10:26 UTC (permalink / raw)
To: Christophe Lyon
Cc: Charles Baylis, Richard Earnshaw, gcc-patches, Marcus Shawcroft,
Tejas Belagod
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
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store
2014-12-09 15:28 [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store charles.baylis
` (3 preceding siblings ...)
2014-12-09 15:28 ` [PATCH 3/4] vstN_lane error message enhancements (Q register) charles.baylis
@ 2014-12-10 10:34 ` Alan Lawrence
2014-12-12 14:12 ` Charles Baylis
4 siblings, 1 reply; 11+ messages in thread
From: Alan Lawrence @ 2014-12-10 10:34 UTC (permalink / raw)
To: charles.baylis
Cc: Richard Earnshaw, gcc-patches, Marcus Shawcroft, Tejas Belagod
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(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store
2014-12-10 10:34 ` [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store Alan Lawrence
@ 2014-12-12 14:12 ` Charles Baylis
0 siblings, 0 replies; 11+ messages in thread
From: Charles Baylis @ 2014-12-12 14:12 UTC (permalink / raw)
To: Alan Lawrence
Cc: Richard Earnshaw, gcc-patches, Marcus Shawcroft, Tejas Belagod
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));
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] vldN_lane error message enhancements (Q registers)
[not found] ` <CAOckXuMSZyHU80A6-VjSzZ7bHrtRikTKwn-45DZ9oPPuRX_0HQ@mail.gmail.com>
@ 2015-04-14 16:30 ` Charles Baylis
2015-04-14 17:20 ` Alan Lawrence
0 siblings, 1 reply; 11+ messages in thread
From: Charles Baylis @ 2015-04-14 16:30 UTC (permalink / raw)
To: Alan Lawrence
Cc: Richard Earnshaw, gcc-patches, Marcus Shawcroft, Tejas Belagod
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] vldN_lane error message enhancements (Q registers)
2015-04-14 16:30 ` Charles Baylis
@ 2015-04-14 17:20 ` Alan Lawrence
0 siblings, 0 replies; 11+ messages in thread
From: Alan Lawrence @ 2015-04-14 17:20 UTC (permalink / raw)
To: Charles Baylis
Cc: Richard Earnshaw, gcc-patches, Marcus Shawcroft, Tejas Belagod
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-04-14 17:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-09 15:28 [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store charles.baylis
2014-12-09 15:28 ` [PATCH 1/4] vldN_lane error message enhancements (Q registers) charles.baylis
[not found] ` <CAOckXuMSZyHU80A6-VjSzZ7bHrtRikTKwn-45DZ9oPPuRX_0HQ@mail.gmail.com>
2015-04-14 16:30 ` Charles Baylis
2015-04-14 17:20 ` Alan Lawrence
2014-12-09 15:28 ` [PATCH 2/4] vldN_lane error message enhancements (D registers) charles.baylis
2014-12-10 9:23 ` Christophe Lyon
2014-12-10 10:26 ` Alan Lawrence
2014-12-09 15:28 ` [PATCH 4/4] vstN_lane " charles.baylis
2014-12-09 15:28 ` [PATCH 3/4] vstN_lane error message enhancements (Q register) charles.baylis
2014-12-10 10:34 ` [PATCH 0/4] [AARCH64,SIMD] PR63870 Improve error messages for single lane load/store Alan Lawrence
2014-12-12 14:12 ` Charles Baylis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).