From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id B52573858D32 for ; Thu, 6 Apr 2023 10:35:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B52573858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6521F16F8; Thu, 6 Apr 2023 03:35:45 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 99A6A3F762; Thu, 6 Apr 2023 03:35:00 -0700 (PDT) From: Richard Sandiford To: Prathamesh Kulkarni Mail-Followup-To: Prathamesh Kulkarni ,Richard Biener , gcc Patches , richard.sandiford@arm.com Cc: Richard Biener , gcc Patches Subject: Re: [aarch64] Use dup and zip1 for interleaving elements in initializing vector References: Date: Thu, 06 Apr 2023 11:34:59 +0100 In-Reply-To: (Prathamesh Kulkarni's message of "Thu, 6 Apr 2023 15:56:55 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-31.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Prathamesh Kulkarni writes: > On Tue, 4 Apr 2023 at 23:35, Richard Sandiford > wrote: >> > diff --git a/gcc/config/aarch64/aarch64-sve-builtins-base.cc b/gcc/config/aarch64/aarch64-sve-builtins-base.cc >> > index cd9cace3c9b..3de79060619 100644 >> > --- a/gcc/config/aarch64/aarch64-sve-builtins-base.cc >> > +++ b/gcc/config/aarch64/aarch64-sve-builtins-base.cc >> > @@ -817,6 +817,62 @@ public: >> > >> > class svdupq_impl : public quiet >> > { >> > +private: >> > + gimple * >> > + fold_nonconst_dupq (gimple_folder &f, unsigned factor) const >> > + { >> > + /* Lower lhs = svdupq (arg0, arg1, ..., argN} into: >> > + tmp = {arg0, arg1, ..., arg} >> > + lhs = VEC_PERM_EXPR (tmp, tmp, {0, 1, 2, N-1, ...}) */ >> > + >> > + /* TODO: Revisit to handle factor by padding zeros. */ >> > + if (factor > 1) >> > + return NULL; >> >> Isn't the key thing here predicate vs. vector rather than factor == 1 vs. >> factor != 1? Do we generate good code for b8, where factor should be 1? > Hi, > It generates the following code for svdup_n_b8: > https://pastebin.com/ypYt590c Hmm, yeah, not pretty :-) But it's not pretty without either. > I suppose lowering to ctor+vec_perm_expr is not really useful > for this case because it won't simplify ctor, unlike the above case of > svdupq_s32 (x[0], x[1], x[2], x[3]); > However I wonder if it's still a good idea to lower svdupq for predicates, for > representing svdupq (or other intrinsics) using GIMPLE constructs as > far as possible ? It's possible, but I think we'd need an example in which its a clear benefit. > In the attached patch, it simply punts if the type > suffix is b, > and doesn't try to fold the call. Yeah, think that's best for now. >> > + >> > + if (BYTES_BIG_ENDIAN) >> > + return NULL; >> > + >> > + tree lhs = gimple_call_lhs (f.call); >> > + if (TREE_CODE (lhs) != SSA_NAME) >> > + return NULL; >> >> Why is this check needed? > This was a left-over from something else I was doing wrongly. Sorry I > forgot to remove it. >> >> > + tree lhs_type = TREE_TYPE (lhs); >> > + tree elt_type = TREE_TYPE (lhs_type); >> > + scalar_mode elt_mode = GET_MODE_INNER (TYPE_MODE (elt_type)); >> >> Aren't we already dealing with a scalar type here? I'd have expected >> SCALAR_TYPE_MODE rather than GET_MODE_INNER (TYPE_MODE ...). > Ugh, sorry, I had most of the code copied over from svld1rq_impl for > building VEC_PERM_EXPR with VLA mask and adjusted it, > but overlooked this :/ >> >> > + machine_mode vq_mode = aarch64_vq_mode (elt_mode).require (); >> > + tree vq_type = build_vector_type_for_mode (elt_type, vq_mode); >> > + >> > + unsigned nargs = gimple_call_num_args (f.call); >> > + vec *v; >> > + vec_alloc (v, nargs); >> > + for (unsigned i = 0; i < nargs; i++) >> > + CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, gimple_call_arg (f.call, i)); >> > + tree vec = build_constructor (vq_type, v); >> > + >> > + tree access_type >> > + = build_aligned_type (vq_type, TYPE_ALIGN (elt_type)); >> >> Nit: seems to fit on one line. But do we need this? We're not accessing >> memory, so I'd have expected vq_type to be OK as-is. >> >> > + tree tmp = make_ssa_name_fn (cfun, access_type, 0); >> > + gimple *g = gimple_build_assign (tmp, vec); >> > + >> > + gimple_seq stmts = NULL; >> > + gimple_seq_add_stmt_without_update (&stmts, g); >> > + >> > + int source_nelts = TYPE_VECTOR_SUBPARTS (access_type).to_constant (); >> >> Looks like we should be able to use nargs instead of source_nelts. > Does the attached patch look OK ? > > Thanks, > Prathamesh >> > >> Thanks, >> Richard >> >> > + poly_uint64 lhs_len = TYPE_VECTOR_SUBPARTS (lhs_type); >> > + vec_perm_builder sel (lhs_len, source_nelts, 1); >> > + for (int i = 0; i < source_nelts; i++) >> > + sel.quick_push (i); >> > + >> > + vec_perm_indices indices (sel, 1, source_nelts); >> > + tree mask_type = build_vector_type (ssizetype, lhs_len); >> > + tree mask = vec_perm_indices_to_tree (mask_type, indices); >> > + >> > + gimple *g2 = gimple_build_assign (lhs, VEC_PERM_EXPR, tmp, tmp, mask); >> > + gimple_seq_add_stmt_without_update (&stmts, g2); >> > + gsi_replace_with_seq (f.gsi, stmts, false); >> > + return g2; >> > + } >> > + >> > public: >> > gimple * >> > fold (gimple_folder &f) const override >> > @@ -832,7 +888,7 @@ public: >> > { >> > tree elt = gimple_call_arg (f.call, i); >> > if (!CONSTANT_CLASS_P (elt)) >> > - return NULL; >> > + return fold_nonconst_dupq (f, factor); >> > builder.quick_push (elt); >> > for (unsigned int j = 1; j < factor; ++j) >> > builder.quick_push (build_zero_cst (TREE_TYPE (vec_type))); >> > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/dupq_11.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/dupq_11.c >> > new file mode 100644 >> > index 00000000000..f19f8deb1e5 >> > --- /dev/null >> > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/dupq_11.c >> > @@ -0,0 +1,31 @@ >> > +/* { dg-do compile } */ >> > +/* { dg-options "-O3 -fdump-tree-optimized" } */ >> > + >> > +#include >> > +#include >> > + >> > +svint8_t f_s8(int8x16_t x) >> > +{ >> > + return svdupq_s8 (x[0], x[1], x[2], x[3], x[4], x[5], x[6], x[7], >> > + x[8], x[9], x[10], x[11], x[12], x[13], x[14], x[15]); >> > +} >> > + >> > +svint16_t f_s16(int16x8_t x) >> > +{ >> > + return svdupq_s16 (x[0], x[1], x[2], x[3], x[4], x[5], x[6], x[7]); >> > +} >> > + >> > +svint32_t f_s32(int32x4_t x) >> > +{ >> > + return svdupq_s32 (x[0], x[1], x[2], x[3]); >> > +} >> > + >> > +svint64_t f_s64(int64x2_t x) >> > +{ >> > + return svdupq_s64 (x[0], x[1]); >> > +} >> > + >> > +/* { dg-final { scan-tree-dump "VEC_PERM_EXPR" "optimized" } } */ >> > +/* { dg-final { scan-tree-dump-not "svdupq" "optimized" } } */ >> > + >> > +/* { dg-final { scan-assembler-times {\tdup\tz[0-9]+\.q, z[0-9]+\.q\[0\]\n} 4 } } */ > > [SVE] Fold svld1rq to VEC_PERM_EXPR if elements are not constant. > > gcc/ChangeLog: > * config/aarch64/aarch64-sve-builtins-base.cc > (svdupq_impl::fold_nonconst_dupq): New method. > (svdupq_impl::fold): Call fold_nonconst_dupq. > > gcc/testsuite/ChangeLog: > * gcc.target/aarch64/sve/acle/general/dupq_11.c: New test. OK for GCC 14, thanks. Richard > diff --git a/gcc/config/aarch64/aarch64-sve-builtins-base.cc b/gcc/config/aarch64/aarch64-sve-builtins-base.cc > index cd9cace3c9b..1732bf8be61 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins-base.cc > +++ b/gcc/config/aarch64/aarch64-sve-builtins-base.cc > @@ -817,6 +817,52 @@ public: > > class svdupq_impl : public quiet > { > +private: > + gimple * > + fold_nonconst_dupq (gimple_folder &f) const > + { > + /* Lower lhs = svdupq (arg0, arg1, ..., argN} into: > + tmp = {arg0, arg1, ..., arg} > + lhs = VEC_PERM_EXPR (tmp, tmp, {0, 1, 2, N-1, ...}) */ > + > + if (f.type_suffix (0).bool_p > + || BYTES_BIG_ENDIAN) > + return NULL; > + > + tree lhs = gimple_call_lhs (f.call); > + tree lhs_type = TREE_TYPE (lhs); > + tree elt_type = TREE_TYPE (lhs_type); > + scalar_mode elt_mode = SCALAR_TYPE_MODE (elt_type); > + machine_mode vq_mode = aarch64_vq_mode (elt_mode).require (); > + tree vq_type = build_vector_type_for_mode (elt_type, vq_mode); > + > + unsigned nargs = gimple_call_num_args (f.call); > + vec *v; > + vec_alloc (v, nargs); > + for (unsigned i = 0; i < nargs; i++) > + CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, gimple_call_arg (f.call, i)); > + tree vec = build_constructor (vq_type, v); > + tree tmp = make_ssa_name_fn (cfun, vq_type, 0); > + gimple *g = gimple_build_assign (tmp, vec); > + > + gimple_seq stmts = NULL; > + gimple_seq_add_stmt_without_update (&stmts, g); > + > + poly_uint64 lhs_len = TYPE_VECTOR_SUBPARTS (lhs_type); > + vec_perm_builder sel (lhs_len, nargs, 1); > + for (unsigned i = 0; i < nargs; i++) > + sel.quick_push (i); > + > + vec_perm_indices indices (sel, 1, nargs); > + tree mask_type = build_vector_type (ssizetype, lhs_len); > + tree mask = vec_perm_indices_to_tree (mask_type, indices); > + > + gimple *g2 = gimple_build_assign (lhs, VEC_PERM_EXPR, tmp, tmp, mask); > + gimple_seq_add_stmt_without_update (&stmts, g2); > + gsi_replace_with_seq (f.gsi, stmts, false); > + return g2; > + } > + > public: > gimple * > fold (gimple_folder &f) const override > @@ -832,7 +878,7 @@ public: > { > tree elt = gimple_call_arg (f.call, i); > if (!CONSTANT_CLASS_P (elt)) > - return NULL; > + return fold_nonconst_dupq (f); > builder.quick_push (elt); > for (unsigned int j = 1; j < factor; ++j) > builder.quick_push (build_zero_cst (TREE_TYPE (vec_type))); > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/dupq_11.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/dupq_11.c > new file mode 100644 > index 00000000000..f19f8deb1e5 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/dupq_11.c > @@ -0,0 +1,31 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -fdump-tree-optimized" } */ > + > +#include > +#include > + > +svint8_t f_s8(int8x16_t x) > +{ > + return svdupq_s8 (x[0], x[1], x[2], x[3], x[4], x[5], x[6], x[7], > + x[8], x[9], x[10], x[11], x[12], x[13], x[14], x[15]); > +} > + > +svint16_t f_s16(int16x8_t x) > +{ > + return svdupq_s16 (x[0], x[1], x[2], x[3], x[4], x[5], x[6], x[7]); > +} > + > +svint32_t f_s32(int32x4_t x) > +{ > + return svdupq_s32 (x[0], x[1], x[2], x[3]); > +} > + > +svint64_t f_s64(int64x2_t x) > +{ > + return svdupq_s64 (x[0], x[1]); > +} > + > +/* { dg-final { scan-tree-dump "VEC_PERM_EXPR" "optimized" } } */ > +/* { dg-final { scan-tree-dump-not "svdupq" "optimized" } } */ > + > +/* { dg-final { scan-assembler-times {\tdup\tz[0-9]+\.q, z[0-9]+\.q\[0\]\n} 4 } } */