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