Re: [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Peter,

on 2023/3/9 07:01, Peter Bergner via Gcc-patches wrote:
> PR109073 shows a problem where GCC 11 and GCC 10 do not accept a const
> __vector_pair pointer operand to some MMA builtins, which GCC 12 and later
> correctly accept.  Fixed here by initializing the builtins to accept const
> pointers.
> 
> This patch was tested in both GCC 11 and GCC 10 on powerpc64le-linux and
> showed no regressions.  Ok for backports?
> 
> Peter
> 
> 
> gcc/
> 
> 	PR target/109073
> 	* config/rs6000/rs6000-call.c (mma_init_builtins): Accept const pointer
> 	operands for lxvp, stxvp and disassemble builtins.
> 
> gcc/testsuite/
> 
> 	PR target/109073
> 	* gcc.target/powerpc/mma-builtin-4.c): New const * test. Update
                                           ~~ typo.

> 	expected instruction counts.
> 	* gcc.target/powerpc/mma-builtin-5.c: Likewise.
> 	* gcc.target/powerpc/mma-builtin-7.c: Likewise.
> 
> 
> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
> index 1be4797e834..3b6d40f0aef 100644
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -14343,22 +14343,30 @@ mma_init_builtins (void)
>  	{
>  	  op[nopnds++] = build_pointer_type (void_type_node);
>  	  if (d->code == MMA_BUILTIN_DISASSEMBLE_ACC)
> -	    op[nopnds++] = build_pointer_type (vector_quad_type_node);
> +	    op[nopnds++] = build_pointer_type (build_qualified_type
> +						 (vector_quad_type_node,
> +						  TYPE_QUAL_CONST));

Nit: Maybe we can build them out of the loop once and then just use the
built one in the loop.

>  	  else
> -	    op[nopnds++] = build_pointer_type (vector_pair_type_node);
> +	    op[nopnds++] = build_pointer_type (build_qualified_type
> +						 (vector_pair_type_node,
> +						  TYPE_QUAL_CONST));
>  	}
>        else if (d->code == VSX_BUILTIN_LXVP)
>  	{
>  	  op[nopnds++] = vector_pair_type_node;
>  	  op[nopnds++] = sizetype;
> -	  op[nopnds++] = build_pointer_type (vector_pair_type_node);
> +	  op[nopnds++] = build_pointer_type (build_qualified_type
> +					       (vector_pair_type_node,
> +						TYPE_QUAL_CONST));
>  	}
>        else if (d->code == VSX_BUILTIN_STXVP)
>  	{
>  	  op[nopnds++] = void_type_node;
>  	  op[nopnds++] = vector_pair_type_node;
>  	  op[nopnds++] = sizetype;
> -	  op[nopnds++] = build_pointer_type (vector_pair_type_node);
> +	  op[nopnds++] = build_pointer_type (build_qualified_type
> +					       (vector_pair_type_node,
> +						TYPE_QUAL_CONST));

I wonder if the bifs which need to be updated are enough here.  The reason why
I asked is that on trunk *ptr_vector_pair_type_node* is used for function types
v1poi_ftype_ulg_pv1poi, v_ftype_pv1poi_uv16qi_uv16qi, v_ftype_pv_pv1poi and
v_ftype_v1poi_ulg_pv1poi, and *ptr_vector_quad_type_node* is used for function
types v_ftype_pv1pxi, v_ftype_pv1pxi_uv16qi_uv16qi, v_ftype_pv1pxi_uv16qi_uv16qi_ci_ci,
v_ftype_pv1pxi_uv16qi_uv16qi_ci_ci_ci, v_ftype_pv1pxi_uv16qi_uv16qi_uv16qi_uv16qi,
v_ftype_pv1pxi_v1poi_uv16qi, v_ftype_pv1pxi_v1poi_uv16qi_ci_ci and v_ftype_pv_pv1pxi.

These function types are further used for bifs as follow:

__builtin_vsx_lxvp
__builtin_mma_assemble_pair
__builtin_vsx_assemble_pair
__builtin_vsx_build_pair
__builtin_mma_disassemble_pair
__builtin_vsx_disassemble_pair
__builtin_vsx_stxvp
__builtin_mma_xxmfacc
__builtin_mma_xxmtacc
__builtin_mma_xxsetaccz
...
... and more ...

Simply testing __builtin_mma_xxmtacc and __builtin_mma_xxmfacc as below:

$ cat test.C
void foo0(const __vector_quad *acc) {
  __builtin_mma_xxmtacc(acc);
  __builtin_mma_xxmfacc(acc);
}

test.C:2:25: error: invalid conversion from ‘const __vector_quad*’ to ‘__vector_quad*’ [-fpermissive]
    2 |   __builtin_mma_xxmtacc(acc);

test.C:3:25: error: invalid conversion from ‘const __vector_quad*’ to ‘__vector_quad*’ [-fpermissive]
    3 |   __builtin_mma_xxmfacc(acc);

They also suffered the same error on gcc11 branch but not on trunk.

Besides, I'm not sure if the existing bif declarations using ptr_vector_pair_type_node
and ptr_vector_quad_type_node are all intentional, at least it looks weird to me that
we declare const __vector_pair* for this __builtin_vsx_stxvp, which is meant to store 32
bytes into the memory provided by the pointer biasing the sizetype offset, but the "const"
qualifier seems to tell that this bif doesn't modify the memory pointed by the given pointer.

As a contrast, for bif vec_xl (a, b), b is the address argument and of type const TYPE *, while for
bif vec_xst (a, b, c), c is the address and of type TYPE *, here we don't add const qualifier for
the address argument of vec_xst as expected.

BR,
Kewen