Re: [PATCH] s390x: Optimize vector permute with constant indexes

[Juergen Christ <jchrist@linux.ibm.com>]

Am Tue, Apr 09, 2024 at 11:51:00AM +0200 schrieb Stefan Schulze Frielinghaus:
> > +static bool expand_perm_as_replicate (const struct expand_vec_perm_d &d)
>                ^~~~~~~~~~~~~~~~~~~~~~~~
> Function names start on a new line.

Fixed

> > +{
> > +  unsigned char i;
> > +  unsigned char elem;
> > +  rtx base = d.op0;
> > +  rtx insn;
> > +  /* Needed to silence maybe-uninitialized warning.  */
> > +  gcc_assert(d.nelt > 0);
>      ~~~~~~~~~~^~~~~~~~~~~~
> Between function name and open bracket whitespace is missing.

Fixed.

> Curiously enough, the error is about d which is a reference and cannot
> be null.  If you are eager you could reduce this and open a PR.
> 
> s390.cc:17935:8: warning: ‘d’ may be used uninitialized [-Wmaybe-uninitialized]
> 17935 |   elem = d.perm[0];
>       |   ~~~~~^~~~~~~~~~~

Weirdly enough it is not `d`, but `d.perm[0]` that seems to be the
problem.  But I did not reduce this.  As the assertion suggests, it is
known that all elements in d.perm in the range [0,d.nelts) are
initialized.  I would like to defer that to a time when I (hopefully)
have some more spare time.

> > +  if (expand_perm_as_replicate(d))
>          ~~~~~~~~~~~~~~~~~~~~~~~~^~~
> Between function name and open bracket whitespace is missing.

Fixed

> > diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-expand-replicate.c b/gcc/testsuite/gcc.target/s390/vector/vec-expand-replicate.c
> > new file mode 100644
> > index 000000000000..27563a00f22b
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/s390/vector/vec-expand-replicate.c
> > @@ -0,0 +1,30 @@
> > +/* Check that the vectorize_vec_perm_const expander correctly deals with
> > +   replication.  Extracted from spec "nab".  */
> > +
> > +/* { dg-do compile } */
> > +/* { dg-options "-O3 -mzarch -march=z13 -fvect-cost-model=unlimited" } */
> > +
> > +
> > +#define REAL_T  double
> > +typedef REAL_T  MATRIX_T[ 4 ][ 4 ];
> > +
> > +int concat_mat_i, concat_mat_j;
> > +static void concat_mat(MATRIX_T m1, MATRIX_T, MATRIX_T m3);
> > +MATRIX_T *rot4p() {
> > +  MATRIX_T mat3, mat4;
> > +  static MATRIX_T mat5;
> > +  concat_mat(mat4, mat3, mat5);
> > +}
> > +void concat_mat(MATRIX_T m1, MATRIX_T, MATRIX_T m3) {
> > +  int k;
> > +  for (;; concat_mat_i++) {
> > +    concat_mat_j = 0;
> > +    for (; 4; concat_mat_j++) {
> > +      k = 0;
> > +      for (; k < 4; k++)
> > +        m3[concat_mat_i][concat_mat_j] += m1[concat_mat_i][k];
> > +    }
> 
> Just nitpicking, if we could come up with a test case which does not
> involve integer overflows due to non-terminating loops, I would prefer
> that.

Well, I have a version without integer overflows, but it still has
non-terminating loops...

Will send a v2,

Juergen