rs6000: Teach rs6000_opaque_type_invalid_use_p about gcall [PR108348]
Commit Message
Hi,
PR108348 shows one special case that MMA opaque types are
used in function arguments and treated as pass by reference,
it results in one copying from argument to a temp variable,
since this copying happens before rs6000_function_arg check,
it can cause ICE without MMA support then. This patch is to
teach function rs6000_opaque_type_invalid_use_p to check if
any function argument in a gcall stmt has the invalid use of
MMA opaque types.
btw, I checked the handling on return value, it doesn't have
this kind of issue as its checking and error emission is quite
early, so this doesn't handle function return value.
Bootstrapped and regtested on powerpc64-linux-gnu P8 and
powerpc64le-linux-gnu P9 and P10.
I'm going to push this soon if no objections.
BR,
Kewen
-----
PR target/108348
gcc/ChangeLog:
* config/rs6000/rs6000.cc (rs6000_opaque_type_invalid_use_p): Add the
support for invalid uses of MMA opaque type in function arguments.
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/pr108348-1.c: New test.
* gcc.target/powerpc/pr108348-2.c: New test.
---
gcc/config/rs6000/rs6000.cc | 19 +++++++++++----
gcc/testsuite/gcc.target/powerpc/pr108348-1.c | 23 +++++++++++++++++++
gcc/testsuite/gcc.target/powerpc/pr108348-2.c | 23 +++++++++++++++++++
3 files changed, 61 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108348-1.c
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108348-2.c
--
2.27.0
Comments
Hi!
On Mon, Jan 16, 2023 at 04:33:36PM +0800, Kewen.Lin wrote:
> PR108348 shows one special case that MMA opaque types are
> used in function arguments and treated as pass by reference,
> it results in one copying from argument to a temp variable,
> since this copying happens before rs6000_function_arg check,
> it can cause ICE without MMA support then. This patch is to
> teach function rs6000_opaque_type_invalid_use_p to check if
> any function argument in a gcall stmt has the invalid use of
> MMA opaque types.
>
> btw, I checked the handling on return value, it doesn't have
> this kind of issue as its checking and error emission is quite
> early, so this doesn't handle function return value.
>
> Bootstrapped and regtested on powerpc64-linux-gnu P8 and
> powerpc64le-linux-gnu P9 and P10.
>
> I'm going to push this soon if no objections.
Looks okay. Some testcase stuff though:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr108348-1.c
> @@ -0,0 +1,23 @@
> +/* { dg-require-effective-target powerpc_p9modulo_ok } */
Please use a saner selector? If one doesn't exist yet, make a new one?
Something that just says "p9", not "modulo".
Thanks,
Segher
Hi Segher,
Thanks for the review comments!
on 2023/1/16 16:49, Segher Boessenkool wrote:
> Hi!
>
> On Mon, Jan 16, 2023 at 04:33:36PM +0800, Kewen.Lin wrote:
>> PR108348 shows one special case that MMA opaque types are
>> used in function arguments and treated as pass by reference,
>> it results in one copying from argument to a temp variable,
>> since this copying happens before rs6000_function_arg check,
>> it can cause ICE without MMA support then. This patch is to
>> teach function rs6000_opaque_type_invalid_use_p to check if
>> any function argument in a gcall stmt has the invalid use of
>> MMA opaque types.
>>
>> btw, I checked the handling on return value, it doesn't have
>> this kind of issue as its checking and error emission is quite
>> early, so this doesn't handle function return value.
>>
>> Bootstrapped and regtested on powerpc64-linux-gnu P8 and
>> powerpc64le-linux-gnu P9 and P10.
>>
>> I'm going to push this soon if no objections.
>
> Looks okay. Some testcase stuff though:
>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr108348-1.c
>> @@ -0,0 +1,23 @@
>> +/* { dg-require-effective-target powerpc_p9modulo_ok } */
>
> Please use a saner selector? If one doesn't exist yet, make a new one?
> Something that just says "p9", not "modulo".
The has_arch_pwr9 looks not suitable here as it doesn't check
the assembler behavior? Do you have some instruction in mind
for being used as the tested instruction mnemonic like modsw?
Maybe later we can deprecate -mmodulo like we get rid of
-mdirect-move, then we can rename p9modulo_ok to p9_ok here? :)
BR,
Kewen
Hi!
On Mon, Jan 16, 2023 at 05:20:56PM +0800, Kewen.Lin wrote:
> on 2023/1/16 16:49, Segher Boessenkool wrote:
> >> +/* { dg-require-effective-target powerpc_p9modulo_ok } */
> >
> > Please use a saner selector? If one doesn't exist yet, make a new one?
> > Something that just says "p9", not "modulo".
>
> The has_arch_pwr9 looks not suitable here as it doesn't check
> the assembler behavior?
? What assembler behaviour?
> Do you have some instruction in mind
> for being used as the tested instruction mnemonic like modsw?
It should not test *any* instruction?
Ah. There is no *_ok wanted or needed at all. You simply are required
to use a new enough binutils to fit your GCC. During development you
probably want something, but that should not end up in the public trees.
You can always use -mcpu=power9. If your toolchain is broken that will
throw some pretty obvious errors your way; this is fine. A user can
always say -mcpu=power9.
The *_ok things should only be used for features that can be disabled
during configuration, or features that we *want* users to be able to
turn off (like FP, VMX, VSX, or HMT or QP float, that kind of thing).
That gives quite enough permutations to test already, we do not need to
create a whole bunch extra for no reason :-)
> Maybe later we can deprecate -mmodulo like we get rid of
> -mdirect-move, then we can rename p9modulo_ok to p9_ok here? :)
We can (and should!) make that compiler flag a stub, yes. But currently
it is used in places as a sneaky way to do -mcpu=power9. So it is a bit
more work :-(
Segher
Hi Segher!
on 2023/1/16 18:40, Segher Boessenkool wrote:
> Hi!
>
> On Mon, Jan 16, 2023 at 05:20:56PM +0800, Kewen.Lin wrote:
>> on 2023/1/16 16:49, Segher Boessenkool wrote:
>>>> +/* { dg-require-effective-target powerpc_p9modulo_ok } */
>>>
>>> Please use a saner selector? If one doesn't exist yet, make a new one?
>>> Something that just says "p9", not "modulo".
>>
>> The has_arch_pwr9 looks not suitable here as it doesn't check
>> the assembler behavior?
>
> ? What assembler behaviour?
I should have said assembler "support", selector powerpc_p9modulo_ok
checks if assembler supports some insn or not, but has_arch_pwr9
doesn't.
>
>> Do you have some instruction in mind
>> for being used as the tested instruction mnemonic like modsw?
>
> It should not test *any* instruction?
>
> Ah. There is no *_ok wanted or needed at all. You simply are required
> to use a new enough binutils to fit your GCC. During development you
> probably want something, but that should not end up in the public trees.
> You can always use -mcpu=power9. If your toolchain is broken that will
> throw some pretty obvious errors your way; this is fine. A user can
> always say -mcpu=power9.
> The *_ok things should only be used for features that can be disabled
> during configuration, or features that we *want* users to be able to
> turn off (like FP, VMX, VSX, or HMT or QP float, that kind of thing).
> That gives quite enough permutations to test already, we do not need to
> create a whole bunch extra for no reason :-)
Thanks for the explanation!! I meant to use powerpc_p9modulo_ok to
exclude those cases where we can't use -mcpu=power9, as you explained we
should not worry about it!? Since the test point requires altivec support
(which is implied when specifying -mcpu=power9, I didn't explicitly specify
it before), I think I could use powerpc_altivec_ok to replace
powerpc_p9modulo_ok here, does it sound good to you?
BR,
Kewen
On Mon, Jan 16, 2023 at 09:05:38PM +0800, Kewen.Lin wrote:
> > The *_ok things should only be used for features that can be disabled
> > during configuration, or features that we *want* users to be able to
> > turn off (like FP, VMX, VSX, or HMT or QP float, that kind of thing).
> > That gives quite enough permutations to test already, we do not need to
> > create a whole bunch extra for no reason :-)
>
> Thanks for the explanation!! I meant to use powerpc_p9modulo_ok to
> exclude those cases where we can't use -mcpu=power9, as you explained we
> should not worry about it!?
But that selector says whether modulo insns are enabled. This is not
correct to use here. I know we have abused these things before, but it
needs to be untangled, not made worse :-)
> Since the test point requires altivec support
> (which is implied when specifying -mcpu=power9, I didn't explicitly specify
> it before), I think I could use powerpc_altivec_ok to replace
> powerpc_p9modulo_ok here, does it sound good to you?
VMX can be turned off even with -mcpu=power9. So yes, it does need
powerpc_altivec_ok. Does it need VSX even?
Segher
Hi Segher,
on 2023/1/16 23:24, Segher Boessenkool wrote:
> On Mon, Jan 16, 2023 at 09:05:38PM +0800, Kewen.Lin wrote:
>>> The *_ok things should only be used for features that can be disabled
>>> during configuration, or features that we *want* users to be able to
>>> turn off (like FP, VMX, VSX, or HMT or QP float, that kind of thing).
>>> That gives quite enough permutations to test already, we do not need to
>>> create a whole bunch extra for no reason :-)
>>
>> Thanks for the explanation!! I meant to use powerpc_p9modulo_ok to
>> exclude those cases where we can't use -mcpu=power9, as you explained we
>> should not worry about it!?
>
> But that selector says whether modulo insns are enabled. This is not
> correct to use here. I know we have abused these things before, but it
> needs to be untangled, not made worse :-)
Makes sense, thanks for further clarifying!
>
>> Since the test point requires altivec support
>> (which is implied when specifying -mcpu=power9, I didn't explicitly specify
>> it before), I think I could use powerpc_altivec_ok to replace
>> powerpc_p9modulo_ok here, does it sound good to you?
>
> VMX can be turned off even with -mcpu=power9. So yes, it does need
> powerpc_altivec_ok. Does it need VSX even?
I think we don't need to check VSX here.
Checking VMX is for robustness and ensure it's consistent with the comment:
/* Allow -maltivec -mabi=no-altivec without warning. Altivec vector
modes only exist for GCC vector types if -maltivec. */
if (TARGET_32BIT && !TARGET_ALTIVEC_ABI && ALTIVEC_VECTOR_MODE (arg.mode))
{
if (TARGET_DEBUG_ARG)
fprintf (stderr, "function_arg_pass_by_reference: AltiVec\n");
return 1;
}
BR,
Kewen
@@ -28922,9 +28922,9 @@ constant_generates_xxspltidp (vec_const_128bit_type *vsx_const)
__vector_pair built-in types. They are target specific and
only available when MMA is supported. With MMA supported, it
simply returns true, otherwise it checks if the given gimple
- STMT is an assignment or asm stmt and uses either of these two
- opaque types unexpectedly, if yes, it would raise an error
- message and returns true, otherwise it returns false. */
+ STMT is an assignment, asm or call stmt and uses either of
+ these two opaque types unexpectedly, if yes, it would raise
+ an error message and returns true, otherwise it returns false. */
bool
rs6000_opaque_type_invalid_use_p (gimple *stmt)
@@ -28953,7 +28953,7 @@ rs6000_opaque_type_invalid_use_p (gimple *stmt)
if (stmt)
{
/* The usage of MMA opaque types is very limited for now,
- to check with gassign and gasm is enough so far. */
+ to check with gassign, gasm and gcall is enough so far. */
if (gassign *ga = dyn_cast<gassign *> (stmt))
{
tree lhs = gimple_assign_lhs (ga);
@@ -28982,6 +28982,17 @@ rs6000_opaque_type_invalid_use_p (gimple *stmt)
return true;
}
}
+ else if (gcall *gc = dyn_cast<gcall *> (stmt))
+ {
+ unsigned nargs = gimple_call_num_args (gc);
+ for (unsigned i = 0; i < nargs; i++)
+ {
+ tree arg = gimple_call_arg (gc, i);
+ tree type = TREE_TYPE (arg);
+ if (check_and_error_invalid_use (type))
+ return true;
+ }
+ }
}
return false;
new file mode 100644
@@ -0,0 +1,23 @@
+/* { dg-require-effective-target powerpc_p9modulo_ok } */
+/* If the default cpu type is power10 or later, type __vector_quad is
+ supported. To keep the test point available all the time, this case
+ specifies -mdejagnu-cpu=power9 here. This needs -mabi=no-altivec
+ to do the copying for pass-by-reference function argument on 32 bit
+ environment. */
+/* { dg-options "-mdejagnu-cpu=power9 -mabi=no-altivec" } */
+
+/* Verify there is no ICE on 32 bit and don't check the error messages
+ on unsupported type since they could be fragile and are not test
+ points of this case. */
+
+/* { dg-excess-errors "pr108348-1" } */
+
+extern void bar (__vector_quad v);
+
+void
+foo (void)
+{
+ __vector_quad v;
+ bar (v);
+}
+
new file mode 100644
@@ -0,0 +1,23 @@
+/* { dg-require-effective-target powerpc_p9modulo_ok } */
+/* If the default cpu type is power10 or later, type __vector_pair is
+ supported. To keep the test point available all the time, this case
+ specifies -mdejagnu-cpu=power9 here. This needs -mabi=no-altivec
+ to do the copying for pass-by-reference function argument on 32 bit
+ environment. */
+/* { dg-options "-mdejagnu-cpu=power9 -mabi=no-altivec" } */
+
+/* Verify there is no ICE on 32 bit and don't check the error messages
+ on unsupported type since they could be fragile and are not test
+ points of this case. */
+
+/* { dg-excess-errors "pr108348-2" } */
+
+extern void bar (__vector_pair v);
+
+void
+foo (void)
+{
+ __vector_pair v;
+ bar (v);
+}
+