* [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]
@ 2023-03-08 23:01 Peter Bergner
2023-03-09 9:30 ` Kewen.Lin
0 siblings, 1 reply; 6+ messages in thread
From: Peter Bergner @ 2023-03-08 23:01 UTC (permalink / raw)
To: GCC Patches; +Cc: Segher Boessenkool, Chip Kerchner
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
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));
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));
}
else
{
diff --git a/gcc/testsuite/gcc.target/powerpc/mma-builtin-4.c b/gcc/testsuite/gcc.target/powerpc/mma-builtin-4.c
index a9fb0107d12..0ba650fcee7 100644
--- a/gcc/testsuite/gcc.target/powerpc/mma-builtin-4.c
+++ b/gcc/testsuite/gcc.target/powerpc/mma-builtin-4.c
@@ -46,6 +46,15 @@ bar2 (vec_t *dst, __vector_pair *src)
dst[4] = res[1];
}
+void
+bar3 (vec_t *dst, const __vector_pair *src)
+{
+ vec_t res[2];
+ __builtin_vsx_disassemble_pair (res, src);
+ dst[0] = res[0];
+ dst[4] = res[1];
+}
+
#if !__has_builtin (__builtin_vsx_assemble_pair)
# error "__has_builtin (__builtin_vsx_assemble_pair) failed"
#endif
@@ -67,7 +76,7 @@ bar2 (vec_t *dst, __vector_pair *src)
#endif
/* { dg-final { scan-assembler-times {\mlxv\M} 6 } } */
-/* { dg-final { scan-assembler-times {\mlxvp\M} 2 } } */
-/* { dg-final { scan-assembler-times {\mstxv\M} 4 } } */
+/* { dg-final { scan-assembler-times {\mlxvp\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mstxv\M} 6 } } */
/* { dg-final { scan-assembler-times {\mstxvp\M} 3 } } */
diff --git a/gcc/testsuite/gcc.target/powerpc/mma-builtin-5.c b/gcc/testsuite/gcc.target/powerpc/mma-builtin-5.c
index 00503b7343d..998c436a8bb 100644
--- a/gcc/testsuite/gcc.target/powerpc/mma-builtin-5.c
+++ b/gcc/testsuite/gcc.target/powerpc/mma-builtin-5.c
@@ -31,6 +31,17 @@ bar (vec_t *dst, __vector_quad *src)
dst[12] = res[3];
}
+void
+bar2 (vec_t *dst, const __vector_quad *src)
+{
+ vec_t res[4];
+ __builtin_mma_disassemble_acc (res, src);
+ dst[0] = res[0];
+ dst[4] = res[1];
+ dst[8] = res[2];
+ dst[12] = res[3];
+}
+
#if !__has_builtin (__builtin_mma_assemble_acc)
# error "__has_builtin (__builtin_mma_assemble_acc) failed"
#endif
@@ -40,8 +51,8 @@ bar (vec_t *dst, __vector_quad *src)
#endif
/* { dg-final { scan-assembler-times {\mlxv\M} 8 } } */
-/* { dg-final { scan-assembler-times {\mlxvp\M} 2 } } */
-/* { dg-final { scan-assembler-times {\mstxv\M} 4 } } */
+/* { dg-final { scan-assembler-times {\mlxvp\M} 4 } } */
+/* { dg-final { scan-assembler-times {\mstxv\M} 8 } } */
/* { dg-final { scan-assembler-times {\mstxvp\M} 4 } } */
-/* { dg-final { scan-assembler-times {\mxxmfacc\M} 3 } } */
-/* { dg-final { scan-assembler-times {\mxxmtacc\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mxxmfacc\M} 4 } } */
+/* { dg-final { scan-assembler-times {\mxxmtacc\M} 4 } } */
diff --git a/gcc/testsuite/gcc.target/powerpc/mma-builtin-7.c b/gcc/testsuite/gcc.target/powerpc/mma-builtin-7.c
index c661a4b84bc..23becfde15e 100644
--- a/gcc/testsuite/gcc.target/powerpc/mma-builtin-7.c
+++ b/gcc/testsuite/gcc.target/powerpc/mma-builtin-7.c
@@ -14,13 +14,25 @@ foo (__vector_pair *dst, __vector_pair *src, long idx)
dst[8] = __builtin_vsx_lxvp (257, src);
}
+void
+bar (__vector_pair *dst, const __vector_pair *src, long idx)
+{
+ dst[0] = __builtin_vsx_lxvp (0, src);
+ dst[2] = __builtin_vsx_lxvp (32, src);
+ dst[4] = __builtin_vsx_lxvp (64, src);
+ /* Non-constant offset should generate a lxvpx. */
+ dst[6] = __builtin_vsx_lxvp (idx, src);
+ /* Non-aligned offset should generate a plxvp. */
+ dst[8] = __builtin_vsx_lxvp (257, src);
+}
+
#if !__has_builtin (__builtin_vsx_lxvp)
# error "__has_builtin (__builtin_vsx_lxvp) failed"
#endif
/* { dg-final { scan-assembler-not {\mlxv\M} } } */
/* { dg-final { scan-assembler-not {\mstxv\M} } } */
-/* { dg-final { scan-assembler-times {\mlxvp\M} 3 } } */
-/* { dg-final { scan-assembler-times {\mlxvpx\M} 1 } } */
-/* { dg-final { scan-assembler-times {\mplxvp\M} 1 } } */
-/* { dg-final { scan-assembler-times {\mstxvp\M} 5 } } */
+/* { dg-final { scan-assembler-times {\mlxvp\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mlxvpx\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mplxvp\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mstxvp\M} 10 } } */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]
2023-03-08 23:01 [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073] Peter Bergner
@ 2023-03-09 9:30 ` Kewen.Lin
2023-03-09 14:55 ` Segher Boessenkool
0 siblings, 1 reply; 6+ messages in thread
From: Kewen.Lin @ 2023-03-09 9:30 UTC (permalink / raw)
To: Peter Bergner; +Cc: Segher Boessenkool, Chip Kerchner, GCC Patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]
2023-03-09 9:30 ` Kewen.Lin
@ 2023-03-09 14:55 ` Segher Boessenkool
2023-03-10 1:24 ` Peter Bergner
2023-03-13 6:55 ` Kewen.Lin
0 siblings, 2 replies; 6+ messages in thread
From: Segher Boessenkool @ 2023-03-09 14:55 UTC (permalink / raw)
To: Kewen.Lin; +Cc: Peter Bergner, Chip Kerchner, GCC Patches
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?
Since the patch is a strict improvement already, it is okay for 11 and
10. But you (Peter) may want to flesh it out a bit first? Or first
commit only this if that works better for you.
Segher
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]
2023-03-09 14:55 ` Segher Boessenkool
@ 2023-03-10 1:24 ` Peter Bergner
2023-03-13 21:25 ` Segher Boessenkool
2023-03-13 6:55 ` Kewen.Lin
1 sibling, 1 reply; 6+ messages in thread
From: Peter Bergner @ 2023-03-10 1:24 UTC (permalink / raw)
To: Segher Boessenkool, Kewen.Lin; +Cc: Chip Kerchner, GCC Patches
On 3/9/23 8:55 AM, Segher Boessenkool wrote:
> 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:
>>> 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.
Correct. I just meant that they're targeted at the two release branches
and not trunk.
>>> 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.
I can have a look at that, but was trying to keep the change as small
as possible. Especially since we're not trying to create code that
will be "easier" to maintain in the future, because this is all changed
in GCC12 with Bill's builtin re-write.
>> 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.
Well I'm sure there are non-MMA builtins that have the same issue.
I was just fixing the ones Chip ran into and similar builtins.
I don't think we want to go and make everything work like it does
on trunk, especially when no one has complained about hitting
them.
As for the __builtin_mma_xxm[ft]acc() errors, I'm not sure any actual
code will ever hit this. All realistic examples declare a __vector_quad
var and the pointer passed to the builtin comes from doing &var as the
operand. Clearly we cannot have a "const __vector_quad var;" and
use that in the builtins.
>> 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.
I'm not a language lawyer and I don't play one on TV. What we're accepting
here, is a pointer with a "const" value that points to non-const memory.
I'll double check the trunk code, but I don't think it allows (and we don't
want it to) using a pointer (const or non-const) that points to a const memory
...at least for the stxvp builtin.
> Since the patch is a strict improvement already, it is okay for 11 and
> 10. But you (Peter) may want to flesh it out a bit first? Or first
> commit only this if that works better for you.
I'll see about making some of the changes above and then I'll report back.
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]
2023-03-10 1:24 ` Peter Bergner
@ 2023-03-13 21:25 ` Segher Boessenkool
0 siblings, 0 replies; 6+ messages in thread
From: Segher Boessenkool @ 2023-03-13 21:25 UTC (permalink / raw)
To: Peter Bergner; +Cc: Kewen.Lin, Chip Kerchner, GCC Patches
Hi!
On Thu, Mar 09, 2023 at 07:24:58PM -0600, Peter Bergner wrote:
> On 3/9/23 8:55 AM, Segher Boessenkool wrote:
> >> 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.
>
> I can have a look at that, but was trying to keep the change as small
> as possible.
Yup, just thinking out loud here, not asking for one way or the other.
> Especially since we're not trying to create code that
> will be "easier" to maintain in the future, because this is all changed
> in GCC12 with Bill's builtin re-write.
Yes :-)
> >> 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.
>
> Well I'm sure there are non-MMA builtins that have the same issue.
Certainly. And if this is only for MMA and it is fixed in 12, then we
can just not deal with it and hope it will work out fine (there are few
users of MMA after all).
> I was just fixing the ones Chip ran into and similar builtins.
> I don't think we want to go and make everything work like it does
> on trunk, especially when no one has complained about hitting
> them.
Right, but we should always (not just here) consider if similar problems
will happen in similar cases. Deciding we don't want to fix this is
just fine, but it helps to have some reasoning behind that.
> I'm not a language lawyer and I don't play one on TV. What we're accepting
> here, is a pointer with a "const" value that points to non-const memory.
Which is perfectly fine. And having a non-const pointer to const data
is fine even (but may well warn)! Trying to actually modify const data
is not.
This is in C; things are different in C++ of course. Evereything is
different in C++.
> I'll double check the trunk code, but I don't think it allows (and we don't
> want it to) using a pointer (const or non-const) that points to a const memory
> ...at least for the stxvp builtin.
That is invalid, sure.
Segher
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073]
2023-03-09 14:55 ` Segher Boessenkool
2023-03-10 1:24 ` Peter Bergner
@ 2023-03-13 6:55 ` Kewen.Lin
1 sibling, 0 replies; 6+ messages in thread
From: Kewen.Lin @ 2023-03-13 6:55 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Peter Bergner, Chip Kerchner, GCC Patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-13 21:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-08 23:01 [PATCH] rs6000: Accept const pointer operands for MMA builtins [PR109073] Peter Bergner
2023-03-09 9:30 ` Kewen.Lin
2023-03-09 14:55 ` Segher Boessenkool
2023-03-10 1:24 ` Peter Bergner
2023-03-13 21:25 ` Segher Boessenkool
2023-03-13 6:55 ` Kewen.Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).