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
