Re: [PATCH 1/5] Add XXSPLTI* and LXVKQ instructions (new data structure and function)

[Michael Meissner <meissner@linux.ibm.com>]

On Fri, Nov 05, 2021 at 12:01:43PM -0500, will schmidt wrote:
> On Fri, 2021-11-05 at 00:04 -0400, Michael Meissner wrote:
> > Add new constant data structure.
> > 
> > This patch provides the data structure and function to convert a
> > CONST_INT, CONST_DOUBLE, CONST_VECTOR, or VEC_DUPLICATE of a constant) to
> > an array of bytes, half-words, words, and  double words that can be loaded
> > into a 128-bit vector register.
> > 
> > The next patches will use this data structure to generate code that
> > generates load of the vector/floating point registers using the XXSPLTIDP,
> > XXSPLTIW, and LXVKQ instructions that were added in power10.
> > 
> > 2021-11-05  Michael Meissner  <meissner@the-meissners.org>
> > 

Whoops, it should be meissner@linux.ibm.com.

> comment to be explicit on the structure name being copied to/from.
> (vec_const_128bit_type is easy to search for, vector or constant or
> structure are not as unique)

Yes, the original name was more generic (rs6000_const).  Originally it could
potentially handle vector constants that were greater than 128-bits if we ever
have support for larger vectors.  But I thought that extra generallity hindered
the code (since you had to check whether the size was exactly 128-bits, etc.).
So I made the data structure tailored to the problem at hand.

> > +
> > +/* Copy an floating point constant to the vector constant structure.  */
> > +
> 
> s/an/a/

Ok.

> > +static void
> > +constant_fp_to_128bit_vector (rtx op,
> > +			      machine_mode mode,
> > +			      size_t byte_num,
> > +			      vec_const_128bit_type *info)
> > +{
> > +  unsigned bitsize = GET_MODE_BITSIZE (mode);
> > +  unsigned num_words = bitsize / 32;
> > +  const REAL_VALUE_TYPE *rtype = CONST_DOUBLE_REAL_VALUE (op);
> > +  long real_words[VECTOR_128BIT_WORDS];
> > +
> > +  /* Make sure we don't overflow the real_words array and that it is
> > +     filled completely.  */
> > +  gcc_assert (num_words <= VECTOR_128BIT_WORDS && (bitsize % 32) == 0);
> 
> Not clear to me on the potential to partially fill the real_words
> array. 

At the moment we don't support a 16-bit floating point type in the compiler
(the Power10 has limited 16-bit floating point support, but we don't make a
special type for it).  If/when we add the 16-bit floating point, we will
possibly need to revisit this.

> > +
> > +  real_to_target (real_words, rtype, mode);
> > +
> > +  /* Iterate over each 32-bit word in the floating point constant.  The
> > +     real_to_target function puts out words in endian fashion.  We need
> 
> Meaning host-endian fashion, or is that meant to be big-endian ? 

Real_to_target puts out the 32-bit values in endian fashion.  This data
structure wants to hold everything in big endian fashion to make checking
things simpler.

> Perhaps also rephrase or move the comment up to indicate that
> real_to_target will have placed or has already placed the words in
> <whatever> endian fashion.
> As stated I was expecting to see a call to real_to_target() below the
> comment. 

Yes, I probably should move the real_to_target call after the comment.

> > +
> > +  /* Possibly splat the constant to fill a vector size.  */
> 
> 
> Suggest "Splat the constant to fill a vector size if ..."

Ok.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com