[PATCH] rs6000: Add new __builtin_vsx_build_pair and __builtin_mma_build_acc built-ins

Segher Boessenkool segher@kernel.crashing.org
Wed Jun 9 21:38:18 GMT 2021


Hi!

On Wed, Jun 09, 2021 at 04:05:37PM -0500, Peter Bergner wrote:
> The __builtin_vsx_assemble_pair and __builtin_mma_assemble_acc built-ins
> currently assign their first source operand to the first VSX register
> in a pair/quad, their second operand to the second register in a pair/quad, etc.
> This is not endian friendly and forces the user to generate different calls
> depending on endianness.  In agreement with the POWER LLVM team, we've
> decided to lightly deprecate the assemble built-ins and replace them with
> "build" built-ins that automatically handle endianness so the same built-in
> call and be used for both little-endian and big-endian compiles.  We are not
> removing the assemble built-ins, since there is code in the wild that use
> them, but we are removing their documentation to encourage the use of the
> new "build" variants.

It is better if you *do* document the old names, but say "use the new
stuff", I think?  Or is there so little material with the old names
out there that no one will notice?

> +      /* The ASSEMBLE builtin source operands are reversed in little-endian
> +	 mode, so reorder them.  */
> +      if (fcode == VSX_BUILTIN_ASSEMBLE_PAIR_INTERNAL && !WORDS_BIG_ENDIAN)
> +	pat = GEN_FCN (icode) (op[0], op[2], op[1]);
> +      else
> +	pat = GEN_FCN (icode) (op[0], op[1], op[2]);

I think this reads simpler as
      /* The ASSEMBLE builtin source operands are reversed in little-endian
	 mode, so reorder them.  */
      if (fcode == VSX_BUILTIN_ASSEMBLE_PAIR_INTERNAL && !WORDS_BIG_ENDIAN)
	std::swap (op[1], op[2]);
      pat = GEN_FCN (icode) (op[0], op[1], op[2]);
do you agree?

> +      /* The ASSEMBLE builtin source operands are reversed in little-endian
> +	 mode, so reorder them.  */
> +      if (fcode == MMA_BUILTIN_ASSEMBLE_ACC_INTERNAL && !WORDS_BIG_ENDIAN)
> +	pat = GEN_FCN (icode) (op[0], op[4], op[3], op[2], op[1]);
> +      else
> +	pat = GEN_FCN (icode) (op[0], op[1], op[2], op[3], op[4]);

And
      /* The ASSEMBLE builtin source operands are reversed in little-endian
	 mode, so reorder them.  */
      if (fcode == MMA_BUILTIN_AS> +SEMBLE_ACC_INTERNAL && !WORDS_BIG_ENDIAN)
	{
	  std::swap (op[1], [op[4]);
	  std::swap (op[2], [op[3]);
	}
      pat = GEN_FCN (icode) (op> +[0], op[1], op[2], op[3], op[4]);
for that then of course.

> @@ -14151,7 +14161,8 @@ mma_init_builtins (void)
>  	      if (gimple_func && mode == XOmode)
>  		op[nopnds++] = build_pointer_type (vector_quad_type_node);
>  	      else if (gimple_func && mode == OOmode

Pleae write the
   && mode == OOmode
on a new line as well then?

> +	      int index = WORDS_BIG_ENDIAN ? i: nvecs - 1 - i;

Space before colon.

Okay for trunk and 11 with at least that space fixed.  Thanks!


Segher


More information about the Gcc-patches mailing list