Since r14-3381 the vec_extract/vec_set idiom used by the vcopy*_lane*
intrinsics is folded to a VEC_PERM_EXPR, so the constant permute
expander decides what to emit for it. For a two-element vector the
result takes one lane from each input, but the patterns that matched it
tied the result to one particular input, and for half the lane
combinations that is the wrong one, so the register allocator has to add
a move. On big endian arm_neon.h's lane flip puts
vcopyq_laneq_u64 (a, 1, b, 1) in that half, so it regressed from one
instruction to two and vect_copy_lane_1.c and vget_set_lane_1.c fail;
little endian has the same problem on lane 0.
Add two patterns for the two-element modes, each offering a second
alternative that ties the destination to the other input, so that the
register allocator can insert into whichever input already occupies the
destination. The first handles a lane copy whose source and destination
lanes are the same; the second handles a lane insert from a scalar,
which always sits in the low part of its register, so it is restricted
to the case where the destination is architectural lane 0. Both sit
before the corresponding general patterns, which continue to handle
every other case, including cross-lane copies.
Both lane combinations of vcopy_lane_{s,u,f}32 and
vcopyq_laneq_{s,u,f}64 now expand to a single INS on both endiannesses.
The double add/sub merge in addsub_{1,2}.c uses the same two-element
merge; with the tie left to the allocator its e1 case now inserts into
lane 0 rather than lane 1, still a single INS, so the expected output is
updated.
PR tree-optimization/123951
gcc/ChangeLog:
* config/aarch64/aarch64-simd.md
(*aarch64_simd_vec_set_lane0<mode>): New pattern.
(*aarch64_simd_vec_copy_lane_same<mode>): New pattern.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/pr123951_1.c: New test.
* gcc.target/aarch64/pr123951_2.c: New test.
* gcc.target/aarch64/simd/addsub_1.c: Update e1 lane insert from
d[1] to d[0].
* gcc.target/aarch64/simd/addsub_2.c: Likewise.
Signed-off-by: Rohith Kapelli <[email protected]>
---
> It looks like both patterns could use the "@..." syntax to specify the
> alternatives, rather than switching on which_alternative. There isn't
> the ENDIAN_LANE_N stuff that makes some of the other patterns require
> C++ code.
>
> The patterns could then use the new alternative syntax, where the
> constraints and attributes are on the same line as the asm.
Done, both of them. You're right that nothing here needs the C++: the
lane numbers in the templates are constants, and the only ENDIAN_LANE_N
use is in the insn condition, which is unchanged. v3 converts both
patterns to the "@" form with the constraints and attributes alongside
each alternative, which also lets the per-operand constraint strings and
the separate set_attr blocks go. 54 added lines rather than 68.
One thing worth flagging: the cons: list names operands 0, 1 and 3, so
the two immediates lose their "i". That is deliberate. Comparing the
genoutput data for v2 and v3, every register and memory constraint is
byte-identical and only the two immediates change to "", which is what
*cmov<mode>_insn already does for the operands its own cons: list skips.
Their predicate is immediate_operand, so "i" was not adding anything.
The generated code is unchanged. I compared the assembly out of the v2
and v3 compilers for the two new tests and for a file covering every
VP_2E mode (s64/u64/f64 and s32/u32/f32, both lane pairs and both
directions) plus the vec_set paths (scalar from an FP register, from a
general register and from memory), on aarch64-linux-gnu and
aarch64_be-linux-gnu: byte-identical in all six comparisons.
Bootstrapped and regression tested on aarch64-unknown-linux-gnu, patched
and unpatched built from clean trees in the same environment, both
stage2 == stage3. Comparing every result line across gcc, g++,
libstdc++, libgomp, libitm and libatomic, the two runs are identical
except for 11 new PASSes, which are the two new tests. No PASS->FAIL.
addsub_{1,2}.c do still need the expectation change: with the patch, e1
comes out as ins v.d[0], v.d[0], so the updated files pass and the
d[1] versions fail on that one function and nothing else.
Also tested aarch64.exp and simd/simd.exp as crosses on both
endiannesses, v2 against v3: all four .sum files identical.
> Otherwise this LGTM, but I'm not sure that I should approve.
Thanks. Tamar, since the two-pattern shape is your suggestion, would you
be happy to confirm the approval?
Thanks,
Rohith
gcc/config/aarch64/aarch64-simd.md | 54 +++++++++++++++++++
gcc/testsuite/gcc.target/aarch64/pr123951_1.c | 41 ++++++++++++++
gcc/testsuite/gcc.target/aarch64/pr123951_2.c | 35 ++++++++++++
.../gcc.target/aarch64/simd/addsub_1.c | 2 +-
.../gcc.target/aarch64/simd/addsub_2.c | 2 +-
5 files changed, 132 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr123951_1.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr123951_2.c
diff --git a/gcc/config/aarch64/aarch64-simd.md
b/gcc/config/aarch64/aarch64-simd.md
index ce51e24da36..80d8474b474 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -1330,6 +1330,32 @@
[(set_attr "type" "neon_logic<q>")]
)
+;; Lane insert for the two-element modes. A scalar source always occupies
+;; the low part of its register, architectural lane 0. When the inserted
+;; element is that same lane, the destination can equally be tied to the
+;; source register and the other lane brought in from operand 3, so offer
+;; that as a second alternative and let the register allocator pick
+;; whichever input already occupies the destination. The remaining
+;; alternatives are those of the general pattern below, so that a scalar
+;; from a general register or from memory is unaffected.
+(define_insn "*aarch64_simd_vec_set_lane0<mode>"
+ [(set (match_operand:VP_2E 0 "register_operand")
+ (vec_merge:VP_2E
+ (vec_duplicate:VP_2E
+ (match_operand:<VEL> 1 "aarch64_simd_nonimmediate_operand"))
+ (match_operand:VP_2E 3 "register_operand")
+ (match_operand:SI 2 "immediate_operand")))]
+ "TARGET_SIMD && INTVAL (operands[2]) == (BYTES_BIG_ENDIAN ? 2 : 1)"
+ ;; In the second alternative the destination is the scalar's own register,
+ ;; which already holds it in lane 0, so the other lane comes from operand 3.
+ {@ [ cons: =0 , 1 , 3 ; attrs: type ]
+ [ w , w , 0 ; neon_ins<q> ] ins\t%0.<Vetype>[0],
%1.<Vetype>[0]
+ [ w , 0 , w ; neon_ins<q> ] ins\t%0.<Vetype>[1],
%3.<Vetype>[1]
+ [ w , ?r , 0 ; neon_from_gp<q> ] ins\t%0.<Vetype>[0],
%<vwcore>1
+ [ w , Utv , 0 ; neon_load1_one_lane<q> ] ld1\t{%0.<Vetype>}[0], %1
+ }
+)
+
(define_insn "@aarch64_simd_vec_set<mode>"
[(set (match_operand:VALL_F16 0 "register_operand" "=w,w,w")
(vec_merge:VALL_F16
@@ -1401,6 +1427,34 @@
}
)
+;; Lane copy between two two-element vectors. When the source and
+;; destination lanes are the same, the copy reads one lane from each input,
+;; so it can equally be done by inserting the live lane of either input into
+;; the other. Offer both directions and let the register allocator tie
+;; whichever input already occupies the destination, so that neither lane
+;; needs an extra move. Other lane combinations are left to the general
+;; pattern below, where only the destination can be tied.
+(define_insn "*aarch64_simd_vec_copy_lane_same<mode>"
+ [(set (match_operand:VP_2E 0 "register_operand")
+ (vec_merge:VP_2E
+ (vec_duplicate:VP_2E
+ (vec_select:<VEL>
+ (match_operand:VP_2E 3 "register_operand")
+ (parallel
+ [(match_operand:SI 4 "immediate_operand")])))
+ (match_operand:VP_2E 1 "register_operand")
+ (match_operand:SI 2 "immediate_operand")))]
+ "TARGET_SIMD
+ && ENDIAN_LANE_N (2, INTVAL (operands[4])) == 1
+ && INTVAL (operands[2]) == (BYTES_BIG_ENDIAN ? 1 : 2)"
+ ;; In the second alternative the destination is operand 3, which already
+ ;; holds lane 1, so lane 0 comes from operand 1.
+ {@ [ cons: =0 , 1 , 3 ; attrs: type ]
+ [ w , 0 , w ; neon_ins<q> ] ins\t%0.<Vetype>[1], %3.<Vetype>[1]
+ [ w , w , 0 ; neon_ins<q> ] ins\t%0.<Vetype>[0], %1.<Vetype>[0]
+ }
+)
+
(define_insn "@aarch64_simd_vec_copy_lane<mode>"
[(set (match_operand:VALL_F16 0 "register_operand" "=w")
(vec_merge:VALL_F16
diff --git a/gcc/testsuite/gcc.target/aarch64/pr123951_1.c
b/gcc/testsuite/gcc.target/aarch64/pr123951_1.c
new file mode 100644
index 00000000000..3c195505a89
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr123951_1.c
@@ -0,0 +1,41 @@
+/* PR tree-optimization/123951. Copying a lane between two vectors must
+ remain a single INS (or ZIP) whichever lane pair is used, on both
+ endiannesses. */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#include <arm_neon.h>
+
+#define BUILD_TEST(TYPE, Q1, Q2, SUFFIX, INDEX1, INDEX2) \
+TYPE __attribute__((noinline,noclone)) \
+test_copy##Q1##_lane##Q2##_##SUFFIX##_##INDEX1##INDEX2 (TYPE a, TYPE b) \
+{ \
+ return vcopy##Q1##_lane##Q2##_##SUFFIX (a, INDEX1, b, INDEX2); \
+}
+
+BUILD_TEST (uint64x2_t, q, q, u64, 0, 0)
+BUILD_TEST (int64x2_t, q, q, s64, 0, 0)
+BUILD_TEST (float64x2_t, q, q, f64, 0, 0)
+/* { dg-final { scan-assembler-times "ins\\tv0.d\\\[0\\\], v1.d\\\[0\\\]" 3 }
} */
+BUILD_TEST (uint64x2_t, q, q, u64, 1, 1)
+BUILD_TEST (int64x2_t, q, q, s64, 1, 1)
+BUILD_TEST (float64x2_t, q, q, f64, 1, 1)
+/* { dg-final { scan-assembler-times "ins\\tv0.d\\\[1\\\], v1.d\\\[1\\\]" 3 }
} */
+BUILD_TEST (uint64x2_t, q, q, u64, 1, 0)
+BUILD_TEST (int64x2_t, q, q, s64, 1, 0)
+BUILD_TEST (float64x2_t, q, q, f64, 1, 0)
+/* { dg-final { scan-assembler-times "zip1\\tv0.2d, v0.2d, v1.2d" 3 } } */
+BUILD_TEST (uint64x2_t, q, q, u64, 0, 1)
+BUILD_TEST (int64x2_t, q, q, s64, 0, 1)
+BUILD_TEST (float64x2_t, q, q, f64, 0, 1)
+/* { dg-final { scan-assembler-times "zip2\\tv0.2d, v1.2d, v0.2d" 3 } } */
+BUILD_TEST (uint32x2_t, , , u32, 0, 0)
+BUILD_TEST (int32x2_t, , , s32, 0, 0)
+BUILD_TEST (float32x2_t, , , f32, 0, 0)
+/* { dg-final { scan-assembler-times "ins\\tv0.s\\\[0\\\], v1.s\\\[0\\\]" 3 }
} */
+BUILD_TEST (uint32x2_t, , , u32, 1, 1)
+BUILD_TEST (int32x2_t, , , s32, 1, 1)
+BUILD_TEST (float32x2_t, , , f32, 1, 1)
+/* { dg-final { scan-assembler-times "ins\\tv0.s\\\[1\\\], v1.s\\\[1\\\]" 3 }
} */
+
+/* { dg-final { scan-assembler-not "\\tmov\\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/pr123951_2.c
b/gcc/testsuite/gcc.target/aarch64/pr123951_2.c
new file mode 100644
index 00000000000..59770b6b000
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr123951_2.c
@@ -0,0 +1,35 @@
+/* PR tree-optimization/123951. Like pr123951_1.c, but for generic vector
+ shuffles, including ones that only become lane inserts after being
+ re-encoded to a wider element mode. */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef unsigned long long v2di __attribute__((vector_size (16)));
+typedef unsigned int v4si __attribute__((vector_size (16)));
+
+v2di
+shuffle_03 (v2di a, v2di b)
+{
+ return __builtin_shuffle (a, b, (v2di) { 0, 3 });
+}
+
+v2di
+shuffle_21 (v2di a, v2di b)
+{
+ return __builtin_shuffle (a, b, (v2di) { 2, 1 });
+}
+
+v4si
+shuffle_0167 (v4si a, v4si b)
+{
+ return __builtin_shuffle (a, b, (v4si) { 0, 1, 6, 7 });
+}
+
+v4si
+shuffle_4523 (v4si a, v4si b)
+{
+ return __builtin_shuffle (a, b, (v4si) { 4, 5, 2, 3 });
+}
+
+/* { dg-final { scan-assembler-times "\\tins\\t" 4 } } */
+/* { dg-final { scan-assembler-not "\\tmov\\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
b/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
index 1fb91a34c42..5acfe33a576 100644
--- a/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/simd/addsub_1.c
@@ -43,7 +43,7 @@ void d1 (_Float16 *restrict a, _Float16 *restrict b, _Float16
*res, int n)
** ...
** fadd v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
** fsub v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
-** ins v[0-9]+.d\[1\], v[0-9]+.d\[1\]
+** ins v[0-9]+.d\[0\], v[0-9]+.d\[0\]
** ...
*/
void e1 (double *restrict a, double *restrict b, double *res, int n)
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
b/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
index 87424c94f24..023adb46ece 100644
--- a/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/simd/addsub_2.c
@@ -43,7 +43,7 @@ void d1 (_Float16 *restrict a, _Float16 *restrict b, _Float16
*res, int n)
** ...
** fsub v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
** fadd v[0-9]+.2d, v[0-9]+.2d, v[0-9]+.2d
-** ins v[0-9]+.d\[1\], v[0-9]+.d\[1\]
+** ins v[0-9]+.d\[0\], v[0-9]+.d\[0\]
** ...
*/
void e1 (double *restrict a, double *restrict b, double *res, int n)
--
2.53.0
