[RFC] __builtin_shuffle sometimes should produce zip1 rather than TBL (PR82199)

The following patch enables vector permutations optimization by using another vector element size when applicable.
It allows usage of simpler instructions in applicable cases.

example:
#define vector __attribute__((vector_size(16) ))

vector float f(vector float a, vector float b)
{
  return __builtin_shuffle  (a, b, (vector int){0, 1, 4,5});
}

was compiled into:
...
	adrp	x0, .LC0
	ldr	q2, [x0, #:lo12:.LC0]
	tbl	v0.16b, {v0.16b - v1.16b}, v2.16b
...

and after patch:
...
	zip1	v0.2d, v0.2d, v1.2d
...

bootstrapped and tested on aarch64-linux-gnu with no regressions


This patch was initially introduced by Andrew Pinksi <apinski@marvell.com> with me being involved later.

(I have no write access to repo)

Thanks,
Dmitrij

gcc/ChangeLog:

2020-06-11	Andrew Pinski	<apinksi@marvell.com>

	PR gcc/82199

	* gcc/config/aarch64/aarch64.c (aarch64_evpc_reencode): New function

gcc/testsuite/ChangeLog:

2020-06-11      Andrew Pinski   <apinksi@marvell.com>

	PR gcc/82199

	* gcc.target/aarch64/vdup_n_3.c: New test
	* gcc.target/aarch64/vzip_1.c: New test
	* gcc.target/aarch64/vzip_2.c: New test
	* gcc.target/aarch64/vzip_3.c: New test
	* gcc.target/aarch64/vzip_4.c: New test

Co-Authored-By:	Dmitrij Pochepko	<dmitrij.pochepko@bell-sw.com>



Thanks,
Dmitrij

Adding AArch64 maintainers.

> -----Original Message-----
> From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Dmitrij
> Pochepko
> Sent: Thursday, June 11, 2020 12:22 PM
> To: gcc-patches@gcc.gnu.org
> Subject: [PATCH][RFC] __builtin_shuffle sometimes should produce zip1
> rather than TBL (PR82199)
> 
> The following patch enables vector permutations optimization by using
> another vector element size when applicable.
> It allows usage of simpler instructions in applicable cases.
> 
> example:
> #define vector __attribute__((vector_size(16) ))
> 
> vector float f(vector float a, vector float b) {
>   return __builtin_shuffle  (a, b, (vector int){0, 1, 4,5}); }
> 
> was compiled into:
> ...
> 	adrp	x0, .LC0
> 	ldr	q2, [x0, #:lo12:.LC0]
> 	tbl	v0.16b, {v0.16b - v1.16b}, v2.16b
> ...
> 
> and after patch:
> ...
> 	zip1	v0.2d, v0.2d, v1.2d
> ...
> 
> bootstrapped and tested on aarch64-linux-gnu with no regressions
> 
> 
> This patch was initially introduced by Andrew Pinksi <apinski@marvell.com>
> with me being involved later.
> 
> (I have no write access to repo)
> 
> Thanks,
> Dmitrij
> 
> gcc/ChangeLog:
> 
> 2020-06-11	Andrew Pinski	<apinksi@marvell.com>
> 
> 	PR gcc/82199
> 
> 	* gcc/config/aarch64/aarch64.c (aarch64_evpc_reencode): New
> function
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-06-11      Andrew Pinski   <apinksi@marvell.com>
> 
> 	PR gcc/82199
> 
> 	* gcc.target/aarch64/vdup_n_3.c: New test
> 	* gcc.target/aarch64/vzip_1.c: New test
> 	* gcc.target/aarch64/vzip_2.c: New test
> 	* gcc.target/aarch64/vzip_3.c: New test
> 	* gcc.target/aarch64/vzip_4.c: New test
> 
> Co-Authored-By:	Dmitrij Pochepko	<dmitrij.pochepko@bell-
> sw.com>
> 
> 
> 
> Thanks,
> Dmitrij

Sorry for the slow review.

Dmitrij Pochepko <dmitrij.pochepko@bell-sw.com> writes:
> @@ -20074,6 +20076,83 @@ aarch64_evpc_trn (struct expand_vec_perm_d *d)
>    return true;
>  }
>  
> +/* Try to re-encode the PERM constant so it use the bigger size up.
> +   This rewrites constants such as {0, 1, 4, 5}/V4SF to {0, 2}/V2DI.
> +   We retry with this new constant with the full suite of patterns.  */
> +static bool
> +aarch64_evpc_reencode (struct expand_vec_perm_d *d)
> +{
> +  expand_vec_perm_d newd;
> +  unsigned HOST_WIDE_INT nelt;
> +
> +  if (d->vec_flags != VEC_ADVSIMD)
> +    return false;
> +
> +  unsigned int encoded_nelts = d->perm.encoding ().encoded_nelts ();
> +  for (unsigned int i = 0; i < encoded_nelts; ++i)
> +    if (!d->perm[i].is_constant ())
> +      return false;
> +
> +  /* to_constant is safe since this routine is specific to Advanced SIMD
> +     vectors.  */
> +  nelt = d->perm.length ().to_constant ();
> +
> +  /* Get the new mode.  Always twice the size of the inner
> +     and half the elements.  */
> +  machine_mode new_mode;
> +  switch (d->vmode)
> +    {
> +    /* 128bit vectors.  */
> +    case E_V4SFmode:
> +    case E_V4SImode:
> +      new_mode = V2DImode;
> +      break;
> +    case E_V8BFmode:
> +    case E_V8HFmode:
> +    case E_V8HImode:
> +      new_mode = V4SImode;
> +      break;
> +    case E_V16QImode:
> +      new_mode = V8HImode;
> +      break;
> +    /* 64bit vectors.  */
> +    case E_V4BFmode:
> +    case E_V4HFmode:
> +    case E_V4HImode:
> +      new_mode = V2SImode;
> +      break;
> +    case E_V8QImode:
> +      new_mode = V4HImode;
> +      break;
> +    default:
> +      return false;
> +    }
> +
> +  newd.vmode = new_mode;
> +  newd.vec_flags = VEC_ADVSIMD;
> +  newd.target = d->target ? gen_lowpart (new_mode, d->target) : NULL;
> +  newd.op0 = d->op0 ? gen_lowpart (new_mode, d->op0) : NULL;
> +  newd.op1 = d->op1 ? gen_lowpart (new_mode, d->op1) : NULL;
> +  newd.testing_p = d->testing_p;
> +  newd.one_vector_p = d->one_vector_p;
> +  vec_perm_builder newpermconst;
> +  newpermconst.new_vector (nelt / 2, nelt / 2, 1);
> +
> +  /* Convert the perm constant if we can.  Require even, odd as the pairs.  */
> +  for (unsigned int i = 0; i < nelt; i += 2)
> +    {
> +      unsigned int elt0 = d->perm[i].to_constant ();
> +      unsigned int elt1 = d->perm[i+1].to_constant ();
> +      if ((elt0 & 1) != 0 || elt0 + 1 != elt1)
> +	return false;
> +      newpermconst.quick_push (elt0 / 2);
> +    }
> +  newpermconst.finalize ();

I think it would be simpler to do it in this order:

  - check for Advanced SIMD, bail out if not
  - get the new mode, bail out if none
  - calculate the permutation vector, bail out if not suitable
  - set up the rest of “newd”

There would then only be one walk over d->perm rather than two,
and we'd only create the gen_lowparts when there's something to test.

The new mode can be calculated with something like:

  poly_uint64 vec_bits = GET_MODE_BITSIZE (d->vmode);
  unsigned int new_elt_bits = GET_MODE_UNIT_BITSIZE (d->vmode) * 2;
  auto new_elt_mode = int_mode_for_size (new_elt_bits, false).require ();
  machine_mode new_mode = aarch64_simd_container_mode (new_elt_mode, vec_bits);

“new_mode” will be “word_mode” on failure.

> diff --git a/gcc/testsuite/gcc.target/aarch64/vdup_n_3.c b/gcc/testsuite/gcc.target/aarch64/vdup_n_3.c
> new file mode 100644
> index 0000000..289604d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vdup_n_3.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(4*sizeof(float))))
> +
> +/* These are both dups. */
> +vector float f(vector float a, vector float b)
> +{
> +  return __builtin_shuffle (a, a, (vector int){0, 1, 0, 1});
> +}
> +vector float f1(vector float a, vector float b)
> +{
> +  return __builtin_shuffle (a, a, (vector int){2, 3, 2, 3});
> +}
> +
> +/* { dg-final { scan-assembler-times "\[ \t\]*dup\[ \t\]+v\[0-9\]+\.2d" 2 } } */

The regexp would be easier to read if quoted using {…}, which requires
fewer backslashes.  Same for the other tests.

Thanks,
Richard

Hi,

thank you for looking into this.

I prepared new patch with all your comments addressed.

Thanks,
Dmitrij

On Tue, Jun 23, 2020 at 05:53:00PM +0100, Richard Sandiford wrote:
...
> 
> I think it would be simpler to do it in this order:
> 
>   - check for Advanced SIMD, bail out if not
>   - get the new mode, bail out if none
>   - calculate the permutation vector, bail out if not suitable
>   - set up the rest of “newd”
> 
> There would then only be one walk over d->perm rather than two,
> and we'd only create the gen_lowparts when there's something to test.
> 
> The new mode can be calculated with something like:
> 
>   poly_uint64 vec_bits = GET_MODE_BITSIZE (d->vmode);
>   unsigned int new_elt_bits = GET_MODE_UNIT_BITSIZE (d->vmode) * 2;
>   auto new_elt_mode = int_mode_for_size (new_elt_bits, false).require ();
>   machine_mode new_mode = aarch64_simd_container_mode (new_elt_mode, vec_bits);
> 
> “new_mode” will be “word_mode” on failure.
>
... 
> The regexp would be easier to read if quoted using {…}, which requires
> fewer backslashes.  Same for the other tests.
> 
> Thanks,
> Richard

Dmitrij Pochepko <dmitrij.pochepko@bell-sw.com> writes:
> Hi,
>
> thank you for looking into this.
>
> I prepared new patch with all your comments addressed.

Thanks, looks good, just a couple of minor things:

> @@ -20090,6 +20092,62 @@ aarch64_evpc_trn (struct expand_vec_perm_d *d)
>    return true;
>  }
>  
> +/* Try to re-encode the PERM constant so it use the bigger size up.

maybe s/use bigger size up/combines odd and even elements/

> +   This rewrites constants such as {0, 1, 4, 5}/V4SF to {0, 2}/V2DI.
> +   We retry with this new constant with the full suite of patterns.  */
> +static bool
> +aarch64_evpc_reencode (struct expand_vec_perm_d *d)
> +{
> +  expand_vec_perm_d newd;
> +  unsigned HOST_WIDE_INT nelt;
> +
> +  if (d->vec_flags != VEC_ADVSIMD)
> +    return false;
> +
> +  /* Get the new mode.  Always twice the size of the inner
> +     and half the elements.  */
> +  poly_uint64 vec_bits = GET_MODE_BITSIZE (d->vmode);
> +  unsigned int new_elt_bits = GET_MODE_UNIT_BITSIZE (d->vmode) * 2;
> +  auto new_elt_mode = int_mode_for_size (new_elt_bits, false).require ();
> +  machine_mode new_mode = aarch64_simd_container_mode (new_elt_mode, vec_bits);
> +
> +  if (new_mode == word_mode)
> +    return false;
> +
> +  /* to_constant is safe since this routine is specific to Advanced SIMD
> +     vectors.  */
> +  nelt = d->perm.length ().to_constant ();
> +
> +  vec_perm_builder newpermconst;
> +  newpermconst.new_vector (nelt / 2, nelt / 2, 1);
> +
> +  /* Convert the perm constant if we can.  Require even, odd as the pairs.  */
> +  for (unsigned int i = 0; i < nelt; i += 2)
> +    {
> +      poly_int64 elt_poly0 = d->perm[i];
> +      poly_int64 elt_poly1 = d->perm[i+1];
> +      if (!elt_poly0.is_constant () || !elt_poly1.is_constant ())
> +	return false;
> +      unsigned int elt0 = elt_poly0.to_constant ();
> +      unsigned int elt1 = elt_poly1.to_constant ();
> +      if ((elt0 & 1) != 0 || elt0 + 1 != elt1)
> +	return false;
> +      newpermconst.quick_push (elt0 / 2);

It should be possible to do this without the to_constants, e.g.:

  poly_int64 elt0 = d->perm[i];
  poly_int64 elt1 = d->perm[i + 1];
  poly_int64 newelt;
  if (!multiple_p (elt0, 2, &newelt) || maybe_ne (elt0 + 1, elt1))
    return false;

(The coding conventions require spaces around “+”, even though I agree
“[i+1]” looks better.)

Looks good otherwise.

Richard

Hi,

please take a look at updated version (attached).

Thanks,
Dmitrij

On Wed, Jul 08, 2020 at 03:48:39PM +0100, Richard Sandiford wrote:
...
> 
> maybe s/use bigger size up/combines odd and even elements/

done

> It should be possible to do this without the to_constants, e.g.:
> 
>   poly_int64 elt0 = d->perm[i];
>   poly_int64 elt1 = d->perm[i + 1];
>   poly_int64 newelt;
>   if (!multiple_p (elt0, 2, &newelt) || maybe_ne (elt0 + 1, elt1))
>     return false;
> 
> (The coding conventions require spaces around “+”, even though I agree
> “[i+1]” looks better.)

done

Dmitrij Pochepko <dmitrij.pochepko@bell-sw.com> writes:
> Hi,
>
> please take a look at updated version (attached).

Looks good, but a couple more points, sorry:

> +  /* Convert the perm constant if we can.  Require even, odd as the pairs.  */
> +  for (unsigned int i = 0; i < nelt; i += 2)
> +    {
> +      poly_int64 elt0 = d->perm[i];
> +      poly_int64 elt1 = d->perm[i + 1];
> +      poly_int64 newelt;
> +      if (!multiple_p (elt0, 2, &newelt) || maybe_ne (elt0 + 1, elt1))
> +	return false;
> +      newpermconst.quick_push (elt0.to_constant () / 2);

This should push “newelt” instead.

> diff --git a/gcc/testsuite/gcc.target/aarch64/vdup_n_3.c b/gcc/testsuite/gcc.target/aarch64/vdup_n_3.c
> new file mode 100644
> index 0000000..5234f5e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vdup_n_3.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(4*sizeof(float))))
> +
> +/* These are both dups. */
> +vector float f(vector float a, vector float b)
> +{
> +  return __builtin_shuffle (a, a, (vector int){0, 1, 0, 1});
> +}
> +vector float f1(vector float a, vector float b)
> +{
> +  return __builtin_shuffle (a, a, (vector int){2, 3, 2, 3});
> +}
> +
> +/* { dg-final { scan-assembler-times {[ \t]*dup[ \t]+v[0-9]+\.2d} 2 } } */

This test is endian-agnostic and should work for all targets, but…

> diff --git a/gcc/testsuite/gcc.target/aarch64/vzip_1.c b/gcc/testsuite/gcc.target/aarch64/vzip_1.c
> new file mode 100644
> index 0000000..5998211
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vzip_1.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(2*sizeof(float))))
> +
> +vector float f(vector float a, vector float b)
> +{
> +  return __builtin_shuffle (a, b, (vector int){0, 2});
> +}
> +
> +/* { dg-final { scan-assembler-times {[ \t]*zip1[ \t]+v[0-9]+\.2s} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/vzip_2.c b/gcc/testsuite/gcc.target/aarch64/vzip_2.c
> new file mode 100644
> index 0000000..2acafde
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vzip_2.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(4*sizeof(float))))
> +
> +vector float f(vector float a, vector float b)
> +{
> +  /* This is the same as zip1 v.2d as {0, 1, 4, 5} can be converted to {0, 2}. */
> +  return __builtin_shuffle (a, b, (vector int){0, 1, 4, 5});
> +}
> +
> +/* { dg-final { scan-assembler-times {[ \t]*zip1[ \t]+v[0-9]+\.2d} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/vzip_3.c b/gcc/testsuite/gcc.target/aarch64/vzip_3.c
> new file mode 100644
> index 0000000..8fa80f7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vzip_3.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(4*sizeof(float))))
> +
> +vector float f(vector float a, vector float b)
> +{
> +  /* This is the same as zip1 v.2d as {4, 5, 0, 1} can be converted to {2, 0}. */
> +  return __builtin_shuffle (a, b, (vector int){4, 5, 0, 1});
> +}
> +
> +/* { dg-final { scan-assembler-times {[ \t]*zip1[ \t]+v[0-9]+\.2d} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/vzip_4.c b/gcc/testsuite/gcc.target/aarch64/vzip_4.c
> new file mode 100644
> index 0000000..aefc55c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/vzip_4.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(4*sizeof(float))))
> +
> +vector float f(vector float a, vector float b)
> +{
> +  /* This is the same as zip2 v.2d as {2, 3, 6, 7} can be converted to {1, 3}. */
> +  return __builtin_shuffle (a, b, (vector int){2, 3, 6, 7});
> +}
> +
> +/* { dg-final { scan-assembler-times {[ \t]*zip2[ \t]+v[0-9]+\.2d} 1 } } */

…the shuffles in these tests are specific to little-endian targets.
For big-endian targets, architectural lane 0 is the last in memory
rather than first, so e.g. the big-endian permute vector for vzip_4.c
would be: { 0, 1, 5, 6 } instead.

I guess the options are:

(1) add __ARM_BIG_ENDIAN preprocessor tests to choose the right mask
(2) restrict the tests to aarch64_little_endian.
(3) have two scan-assembler lines with opposite zip1/zip2 choices, e.g:

/* { dg-final { scan-assembler-times {[ \t]*zip1[ \t]+v[0-9]+\.2d} 1 { target aarch64_big_endian } } } */
/* { dg-final { scan-assembler-times {[ \t]*zip2[ \t]+v[0-9]+\.2d} 1 { target aarch64_little_endian } } } */

(untested).

I guess (3) is probably best, but (2) is fine too if you're not set
up to test big-endian.

Sorry for not noticing last time.  I only realised when testing the
patch locally.

Thanks,
Richard

Hi,

I please take a look at new version (attached).

Thanks,
Dmitrij

On Sat, Jul 11, 2020 at 09:39:13AM +0100, Richard Sandiford wrote:
...
> This should push “newelt” instead.
done

...
> This test is endian-agnostic and should work for all targets, but…
...
> 
> …the shuffles in these tests are specific to little-endian targets.
> For big-endian targets, architectural lane 0 is the last in memory
> rather than first, so e.g. the big-endian permute vector for vzip_4.c
> would be: { 0, 1, 5, 6 } instead.
> 
> I guess the options are:
> 
> (1) add __ARM_BIG_ENDIAN preprocessor tests to choose the right mask
> (2) restrict the tests to aarch64_little_endian.
> (3) have two scan-assembler lines with opposite zip1/zip2 choices, e.g:
> 
> /* { dg-final { scan-assembler-times {[ \t]*zip1[ \t]+v[0-9]+\.2d} 1 { target aarch64_big_endian } } } */
> /* { dg-final { scan-assembler-times {[ \t]*zip2[ \t]+v[0-9]+\.2d} 1 { target aarch64_little_endian } } } */
> 
> (untested).
> 
> I guess (3) is probably best, but (2) is fine too if you're not set
> up to test big-endian.
> 
> Sorry for not noticing last time.  I only realised when testing the
> patch locally.
> 
> Thanks,
> Richard

I restricted tests to aarch64_little_endian, because I don't have big-endian setup to check it.

Dmitrij Pochepko <dmitrij.pochepko@bell-sw.com> writes:
> Hi,
>
> I please take a look at new version (attached).

Thanks, pushed to master with a slightly tweaked changelog.

Richard

Thank you!

On 17.07.2020 12:21, Richard Sandiford wrote:
> Dmitrij Pochepko <dmitrij.pochepko@bell-sw.com> writes:
>> Hi,
>>
>> I please take a look at new version (attached).
> Thanks, pushed to master with a slightly tweaked changelog.
>
> Richard