On Fri, Nov 5, 2021 at 2:13 PM Michael Meissner <meiss...@linux.ibm.com> wrote:
>
> 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 <meiss...@the-meissners.org>
> > >
>
> Whoops, it should be meiss...@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.
Okay.
Thanks, David
