Re: [1/2] PR96463 - aarch64 specific changes

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 8655 bytes --]

On Fri, 6 May 2022 at 16:00, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > diff --git a/gcc/config/aarch64/aarch64-sve-builtins-base.cc b/gcc/config/aarch64/aarch64-sve-builtins-base.cc
> > index c24c0548724..1ef4ea2087b 100644
> > --- a/gcc/config/aarch64/aarch64-sve-builtins-base.cc
> > +++ b/gcc/config/aarch64/aarch64-sve-builtins-base.cc
> > @@ -44,6 +44,14 @@
> >  #include "aarch64-sve-builtins-shapes.h"
> >  #include "aarch64-sve-builtins-base.h"
> >  #include "aarch64-sve-builtins-functions.h"
> > +#include "aarch64-builtins.h"
> > +#include "gimple-ssa.h"
> > +#include "tree-phinodes.h"
> > +#include "tree-ssa-operands.h"
> > +#include "ssa-iterators.h"
> > +#include "stringpool.h"
> > +#include "value-range.h"
> > +#include "tree-ssanames.h"
>
> Minor, but: I think the preferred approach is to include "ssa.h"
> rather than include some of these headers directly.
>
> >
> >  using namespace aarch64_sve;
> >
> > @@ -1207,6 +1215,56 @@ public:
> >      insn_code icode = code_for_aarch64_sve_ld1rq (e.vector_mode (0));
> >      return e.use_contiguous_load_insn (icode);
> >    }
> > +
> > +  gimple *
> > +  fold (gimple_folder &f) const OVERRIDE
> > +  {
> > +    tree arg0 = gimple_call_arg (f.call, 0);
> > +    tree arg1 = gimple_call_arg (f.call, 1);
> > +
> > +    /* Transform:
> > +       lhs = svld1rq ({-1, -1, ... }, arg1)
> > +       into:
> > +       tmp = mem_ref<int32x4_t> [(int * {ref-all}) arg1]
> > +       lhs = vec_perm_expr<tmp, tmp, {0, 1, 2, 3, ...}>.
> > +       on little endian target.  */
> > +
> > +    if (!BYTES_BIG_ENDIAN
> > +     && integer_all_onesp (arg0))
> > +      {
> > +     tree lhs = gimple_call_lhs (f.call);
> > +     auto simd_type = aarch64_get_simd_info_for_type (Int32x4_t);
>
> Does this work for other element sizes?  I would have expected it
> to be the (128-bit) Advanced SIMD vector associated with the same
> element type as the SVE vector.
>
> The testcase should cover more than just int32x4_t -> svint32_t,
> just to be sure.
In the attached patch, it obtains corresponding advsimd type with:

tree eltype = TREE_TYPE (lhs_type);
unsigned nunits = 128 / TREE_INT_CST_LOW (TYPE_SIZE (eltype));
tree vectype = build_vector_type (eltype, nunits);

While this seems to work with different element sizes, I am not sure if it's
the correct approach ?
>
> > +
> > +     tree elt_ptr_type
> > +       = build_pointer_type_for_mode (simd_type.eltype, VOIDmode, true);
> > +     tree zero = build_zero_cst (elt_ptr_type);
> > +
> > +     /* Use element type alignment.  */
> > +     tree access_type
> > +       = build_aligned_type (simd_type.itype, TYPE_ALIGN (simd_type.eltype));
> > +
> > +     tree tmp = make_ssa_name_fn (cfun, access_type, 0);
> > +     gimple *mem_ref_stmt
> > +       = gimple_build_assign (tmp, fold_build2 (MEM_REF, access_type, arg1, zero));
>
> Long line.  Might be easier to format by assigning the fold_build2 result
> to a temporary variable.
>
> > +     gsi_insert_before (f.gsi, mem_ref_stmt, GSI_SAME_STMT);
> > +
> > +     tree mem_ref_lhs = gimple_get_lhs (mem_ref_stmt);
> > +     tree vectype = TREE_TYPE (mem_ref_lhs);
> > +     tree lhs_type = TREE_TYPE (lhs);
>
> Is this necessary?  The code above supplied the types and I wouldn't
> have expected them to change during the build process.
>
> > +
> > +     int source_nelts = TYPE_VECTOR_SUBPARTS (vectype).to_constant ();
> > +     vec_perm_builder sel (TYPE_VECTOR_SUBPARTS (lhs_type), source_nelts, 1);
> > +     for (int i = 0; i < source_nelts; i++)
> > +       sel.quick_push (i);
> > +
> > +     vec_perm_indices indices (sel, 1, source_nelts);
> > +     gcc_checking_assert (can_vec_perm_const_p (TYPE_MODE (lhs_type), indices));
> > +     tree mask = vec_perm_indices_to_tree (lhs_type, indices);
> > +     return gimple_build_assign (lhs, VEC_PERM_EXPR, mem_ref_lhs, mem_ref_lhs, mask);
>
> Nit: long line.
>
> > +      }
> > +
> > +    return NULL;
> > +  }
> >  };
> >
> >  class svld1ro_impl : public load_replicate
> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > index f650abbc4ce..47810fec804 100644
> > --- a/gcc/config/aarch64/aarch64.cc
> > +++ b/gcc/config/aarch64/aarch64.cc
> > @@ -23969,6 +23969,35 @@ aarch64_evpc_sve_tbl (struct expand_vec_perm_d *d)
> >    return true;
> >  }
> >
> > +/* Try to implement D using SVE dup instruction.  */
> > +
> > +static bool
> > +aarch64_evpc_sve_dup (struct expand_vec_perm_d *d)
> > +{
> > +  if (BYTES_BIG_ENDIAN
> > +      || d->perm.length ().is_constant ()
> > +      || !d->one_vector_p
> > +      || d->target == NULL
> > +      || d->op0 == NULL
>
> These last two lines mean that we always return false for d->testing.
> The idea instead is that the return value should be the same for both
> d->testing and !d->testing.  The difference is that for !d->testing we
> also emit code to do the permute.
>
> > +      || GET_MODE_NUNITS (GET_MODE (d->target)).is_constant ()
>
> Sorry, I've forgotten the context now, but: these positive tests
> for is_constant surprised me.  Do we really only want to do this
> for variable-length SVE code generation, rather than fixed-length?
>
> > +      || !GET_MODE_NUNITS (GET_MODE (d->op0)).is_constant ())
> > +    return false;
> > +
> > +  if (d->testing_p)
> > +    return true;
>
> This should happen after the later tests, once we're sure that the
> permute vector has the right form.  If the issue is that op0 isn't
> provided for testing then I think the hook needs to be passed the
> input mode alongside the result mode.
>
> It might then be better to test:
>
>   aarch64_classify_vector_mode (...input_mode...) == VEC_ADVSIMD
>
> (despite what I said earlier, about testing is_constant, sorry).
Thanks for the suggestions, I tried to address them in the attached patch.
Does it look OK after bootstrap+test ?

The patch seems to generate the same code for different vector types.
For eg:

svint32_t foo (int32x4_t x)
{
  return svld1rq (svptrue_b8 (), &x[0]);
}

svint16_t foo2(int16x8_t x)
{
  return svld1rq_s16 (svptrue_b8 (), &x[0]);
}

.optimized dump:
;; Function foo (foo, funcdef_no=4350, decl_uid=29928,
cgraph_uid=4351, symbol_order=4350)
svint32_t foo (int32x4_t x)
{
  svint32_t _2;

  <bb 2> [local count: 1073741824]:
  _2 = VEC_PERM_EXPR <x_3(D), x_3(D), { 0, 1, 2, 3, ... }>;
  return _2;

}

;; Function foo2 (foo2, funcdef_no=4351, decl_uid=29931,
cgraph_uid=4352, symbol_order=4351)

svint16_t foo2 (int16x8_t x)
{
  svint16_t _2;

  <bb 2> [local count: 1073741824]:
  _2 = VEC_PERM_EXPR <x_3(D), x_3(D), { 0, 1, 2, 3, 4, 5, 6, 7, ... }>;
  return _2;

}

resulting in code-gen:
foo:
        dup     z0.q, z0.q[0]
        ret

foo2:
        dup     z0.q, z0.q[0]
        ret

I suppose this is correct, since in both cases it's replicating the
entire 128-bit vector (irrespective of element sizes) ?

Thanks,
Prathamesh
>
> > +
> > +  int npatterns = d->perm.encoding ().npatterns ();
> > +  if (!known_eq (npatterns, GET_MODE_NUNITS (GET_MODE (d->op0))))
> > +    return false;
> > +
> > +  for (int i = 0; i < npatterns; i++)
> > +    if (!known_eq (d->perm[i], i))
> > +      return false;
> > +
> > +  aarch64_expand_sve_dupq (d->target, GET_MODE (d->target), d->op0);
> > +  return true;
> > +}
> > +
> >  /* Try to implement D using SVE SEL instruction.  */
> >
> >  static bool
> > @@ -24129,7 +24158,12 @@ aarch64_expand_vec_perm_const_1 (struct expand_vec_perm_d *d)
> >        else if (aarch64_evpc_reencode (d))
> >       return true;
> >        if (d->vec_flags == VEC_SVE_DATA)
> > -     return aarch64_evpc_sve_tbl (d);
> > +     {
> > +       if (aarch64_evpc_sve_dup (d))
> > +         return true;
> > +       else if (aarch64_evpc_sve_tbl (d))
> > +         return true;
> > +     }
> >        else if (d->vec_flags == VEC_ADVSIMD)
> >       return aarch64_evpc_tbl (d);
> >      }
> > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463.c
> > new file mode 100644
> > index 00000000000..35100a9e01c
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463.c
> > @@ -0,0 +1,17 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O3" } */
> > +
> > +#include "arm_neon.h"
> > +#include "arm_sve.h"
> > +
> > +svint32_t f1 (int32x4_t x)
> > +{
> > +  return svld1rq (svptrue_b8 (), &x[0]);
> > +}
> > +
> > +svint32_t f2 (int *x)
> > +{
> > +  return svld1rq (svptrue_b8 (), x);
> > +}
> > +
> > +/* { dg-final { scan-assembler-times {\tdup\tz[0-9]+\.q, z[0-9]+\.q\[0\]} 2 { target aarch64_little_endian } } } */

[-- Attachment #2: pr96463-8.txt --]
[-- Type: text/plain, Size: 5886 bytes --]

diff --git a/gcc/config/aarch64/aarch64-sve-builtins-base.cc b/gcc/config/aarch64/aarch64-sve-builtins-base.cc
index c24c0548724..8a2e5b886e4 100644
--- a/gcc/config/aarch64/aarch64-sve-builtins-base.cc
+++ b/gcc/config/aarch64/aarch64-sve-builtins-base.cc
@@ -44,6 +44,7 @@
 #include "aarch64-sve-builtins-shapes.h"
 #include "aarch64-sve-builtins-base.h"
 #include "aarch64-sve-builtins-functions.h"
+#include "ssa.h"
 
 using namespace aarch64_sve;
 
@@ -1207,6 +1208,59 @@ public:
     insn_code icode = code_for_aarch64_sve_ld1rq (e.vector_mode (0));
     return e.use_contiguous_load_insn (icode);
   }
+
+  gimple *
+  fold (gimple_folder &f) const OVERRIDE
+  {
+    tree arg0 = gimple_call_arg (f.call, 0);
+    tree arg1 = gimple_call_arg (f.call, 1);
+
+    /* Transform:
+       lhs = svld1rq ({-1, -1, ... }, arg1)
+       into:
+       tmp = mem_ref<vectype> [(int * {ref-all}) arg1]
+       lhs = vec_perm_expr<tmp, tmp, {0, 1, 2, 3, ...}>.
+       on little endian target.
+       vectype is the corresponding ADVSIMD type.  */
+
+    if (!BYTES_BIG_ENDIAN
+	&& integer_all_onesp (arg0))
+      {
+	tree lhs = gimple_call_lhs (f.call);
+	tree lhs_type = TREE_TYPE (lhs);
+	tree eltype = TREE_TYPE (lhs_type);
+	unsigned nunits = 128 / TREE_INT_CST_LOW (TYPE_SIZE (eltype));
+	tree vectype = build_vector_type (eltype, nunits);
+
+	tree elt_ptr_type
+	  = build_pointer_type_for_mode (eltype, VOIDmode, true);
+	tree zero = build_zero_cst (elt_ptr_type);
+
+	/* Use element type alignment.  */
+	tree access_type
+	  = build_aligned_type (vectype, TYPE_ALIGN (eltype));
+
+	tree mem_ref_lhs = make_ssa_name_fn (cfun, access_type, 0);
+	tree mem_ref_op = fold_build2 (MEM_REF, access_type, arg1, zero);
+	gimple *mem_ref_stmt
+	  = gimple_build_assign (mem_ref_lhs, mem_ref_op);
+	gsi_insert_before (f.gsi, mem_ref_stmt, GSI_SAME_STMT);
+
+	int source_nelts = TYPE_VECTOR_SUBPARTS (access_type).to_constant ();
+	vec_perm_builder sel (TYPE_VECTOR_SUBPARTS (lhs_type), source_nelts, 1);
+	for (int i = 0; i < source_nelts; i++)
+	  sel.quick_push (i);
+
+	vec_perm_indices indices (sel, 1, source_nelts);
+	gcc_checking_assert (can_vec_perm_const_p (TYPE_MODE (lhs_type),
+						   indices));
+	tree mask = vec_perm_indices_to_tree (lhs_type, indices);
+	return gimple_build_assign (lhs, VEC_PERM_EXPR,
+				    mem_ref_lhs, mem_ref_lhs, mask);
+      }
+
+    return NULL;
+  }
 };
 
 class svld1ro_impl : public load_replicate
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index f650abbc4ce..072ec9bd153 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -23969,6 +23969,35 @@ aarch64_evpc_sve_tbl (struct expand_vec_perm_d *d)
   return true;
 }
 
+/* Try to implement D using SVE dup instruction.  */
+
+static bool
+aarch64_evpc_sve_dup (struct expand_vec_perm_d *d)
+{
+  if (BYTES_BIG_ENDIAN
+      || d->perm.length ().is_constant ()
+      || !d->one_vector_p
+      || d->target == NULL
+      || d->op0 == NULL
+      || (aarch64_classify_vector_mode (GET_MODE (d->target)) & VEC_ANY_SVE) == 0
+      || (aarch64_classify_vector_mode (GET_MODE (d->op0)) & VEC_ADVSIMD) == 0)
+    return false;
+
+  int npatterns = d->perm.encoding ().npatterns ();
+  if (!known_eq (npatterns, GET_MODE_NUNITS (GET_MODE (d->op0))))
+    return false;
+
+  for (int i = 0; i < npatterns; i++)
+    if (!known_eq (d->perm[i], i))
+      return false;
+
+  if (d->testing_p)
+    return true;
+
+  aarch64_expand_sve_dupq (d->target, GET_MODE (d->target), d->op0);
+  return true;
+}
+
 /* Try to implement D using SVE SEL instruction.  */
 
 static bool
@@ -24129,7 +24158,12 @@ aarch64_expand_vec_perm_const_1 (struct expand_vec_perm_d *d)
       else if (aarch64_evpc_reencode (d))
 	return true;
       if (d->vec_flags == VEC_SVE_DATA)
-	return aarch64_evpc_sve_tbl (d);
+	{
+	  if (aarch64_evpc_sve_dup (d))
+	    return true;
+	  else if (aarch64_evpc_sve_tbl (d))
+	    return true;
+	}
       else if (d->vec_flags == VEC_ADVSIMD)
 	return aarch64_evpc_tbl (d);
     }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463-1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463-1.c
new file mode 100644
index 00000000000..5af3b6ed24c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463-1.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+#include "arm_neon.h"
+#include "arm_sve.h"
+
+#define TEST(ret_type, param_type, suffix) \
+ret_type test_##suffix(param_type x) \
+{ \
+  return svld1rq_##suffix (svptrue_b8 (), &x[0]); \
+}
+
+TEST(svint8_t, int8x16_t, s8)
+TEST(svint16_t, int16x8_t, s16)
+TEST(svint32_t, int32x4_t, s32)
+TEST(svint64_t, int64x2_t, s64)
+
+TEST(svuint8_t, uint8x16_t, u8)
+TEST(svuint16_t, uint16x8_t, u16)
+TEST(svuint32_t, uint32x4_t, u32)
+TEST(svuint64_t, uint64x2_t, u64)
+
+/* { dg-final { scan-assembler-times {\tdup\tz[0-9]+\.q, z[0-9]+\.q\[0\]} 8 { target aarch64_little_endian } } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463-2.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463-2.c
new file mode 100644
index 00000000000..17e78c57c1b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr96463-2.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+#include "arm_neon.h"
+#include "arm_sve.h"
+
+#define TEST(ret_type, param_type, suffix) \
+ret_type test_##suffix(param_type *x) \
+{ \
+  return svld1rq_##suffix (svptrue_b8 (), &x[0]); \
+}
+
+TEST(svint8_t, int8_t, s8)
+TEST(svint16_t, int16_t, s16)
+TEST(svint32_t, int32_t, s32)
+TEST(svint64_t, int64_t, s64)
+
+TEST(svuint8_t, uint8_t, u8)
+TEST(svuint16_t, uint16_t, u16)
+TEST(svuint32_t, uint32_t, u32)
+TEST(svuint64_t, uint64_t, u64)
+
+/* { dg-final { scan-assembler-times {\tdup\tz[0-9]+\.q, z[0-9]+\.q\[0\]} 8 { target aarch64_little_endian } } } */