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

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

Hi Segher,

on 2023/3/9 22:55, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Mar 09, 2023 at 05:30:53PM +0800, Kewen.Lin wrote:
>> 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.
> 
> "Pointers to const" is the more correct.  A "const pointer" is e.g.
>   int *const p;
> not the same thing at all, and sometimes this is useful to have ;-)
> 
>>> This patch was tested in both GCC 11 and GCC 10 on powerpc64le-linux and
>>> showed no regressions.  Ok for backports?
> 
> It isn't truly a backport. You can put it on 11 and 10 at the same time,
> there is no benefit doing it on 11 only first.
> 
>>>  	{
>>>  	  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.
> 
> Or as globals even.  Currently we have X and pointer to X, but no
> pointer to const X (and no const X either, but that isn't so useful).
> 
> The generic code doesn't have this either, hrm.
> 
> (snip)
> 
>> 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.
> 
> Yeah, there is more to be done here.
> 
>> 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.
> 
> That looks like a bug.  Well it is one even.  Is it fixed on trunk?


For the test case (test.c)
---
#include <altivec.h>

void
foo (const __vector_pair *dst, __vector_pair *src, long idx)
{
  __builtin_vsx_stxvp (*src, idx, dst);
}

void
bar (const unsigned char *dst, vector unsigned char *src, long idx)
{
  vec_xst (*src, idx, dst);
}
---

With *gcc-12 or trunk* (either cfe or c++fe), there is no warnings.

With gcc-11:

*cfe*

test.c: In function ‘foo’:
test.c:6:35: warning: passing argument 3 of ‘__builtin_vsx_stxvp’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    6 |   __builtin_vsx_stxvp (*src, idx, dst);
      |                                   ^~~
test.c:6:35: note: expected ‘__vector_pair *’ but argument is of type ‘const __vector_pair *’
test.c: In function ‘bar’:
test.c:12:3: warning: passing argument 3 of ‘__builtin_vec_vsx_st’ discards qualifiers from pointer target type
   12 |   vec_xst (*src, idx, dst);
      |   ^~~~~~~

*c++fe*

test.c: In function ‘void foo(const __vector_pair*, __vector_pair*, long int)’:
test.c:6:35: error: invalid conversion from ‘const __vector_pair*’ to ‘__vector_pair*’ [-fpermissive]
    6 |   __builtin_vsx_stxvp (*src, idx, dst);
      |                                   ^~~
      |                                   |
      |                                   const __vector_pair*
<built-in>: note:   initializing argument 3 of ‘void __builtin_vsx_stxvp(__vector_pair, sizetype, __vector_pair*)’
test.c: In function ‘void bar(const unsigned char*, __vector unsigned char*, long int)’:
test.c:12:11: warning: passing argument 3 of ‘__builtin_vec_vsx_st’ discards qualifiers from pointer target type
   12 |   vec_xst (*src, idx, dst);
      |           ^

So for vec_xst, on gcc-11 which doesn't have new bif framework, there is always a warning.  It
looks that the new bif framework always makes those bif argument pointer types with const
qualifier.  A quick scanning on function rs6000_init_builtins (trunk code) didn't find a counter
case.  The difference on pointers of types __vector_pair* and vector unsigned char* made me wonder
if the const qualifier on __builtin_vsx_stxvp is intentional, maybe opaque types are different?
since we get warning for vec_xst but get error for __builtin_vsx_stxvp even without new bif
framework.

BR,
Kewen