* RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
@ 2020-08-18 3:37 qiaopeixin
2020-08-18 12:18 ` Christophe Lyon
2020-08-18 17:00 ` Richard Sandiford
0 siblings, 2 replies; 11+ messages in thread
From: qiaopeixin @ 2020-08-18 3:37 UTC (permalink / raw)
To: Richard Sandiford, Christophe Lyon; +Cc: gcc-patches
Hi Richard,
Thanks for the review and explanation.
The previous fix adding if condition of TARGET_FLOAT does crash glibc-2.29.
I checked the past log of writing the function aarch64_init_cumulative_args, and did not find the reason why Alan Lawrence added TREE_PUBLIC (fndecl) as one condition for entering the function type check. Maybe Alan could clarify? I tried to delete TREE_PUBLIC (fndecl), which turns out could solve both the glibc problem and the previous ICE problem. A new fix is made as following, passed bootstrap and deja test. I believe this fix is reasonable, since the function type should be checked no matter if it has external linkage or not.
The function aarch64_init_cumulative_args checks the function types and should catch the error that "-mgeneral-regs-only" is incompatible with the use of SIMD/FP registers. In the test case on PR96479, the function myfunc2 returns one vector of 4 integers, while it is defined static type. TREE_PUBLIC (fndecl) is set as false and it prevents from entering if statement and checking function types. I delete "TREE_PUBLIC (fndecl)" so that gcc can catch the error through the function aarch64_init_cumulative_args now. The ICE on PR96479 can report the diagnostic error with this fix. The patch for the fix is attached as following:
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index b7f5bc76f1b..9ce83dce131 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -6017,7 +6017,7 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS *pcum,
if (!silent_p
&& !TARGET_FLOAT
- && fndecl && TREE_PUBLIC (fndecl)
+ && fndecl
&& fntype && fntype != error_mark_node)
{
const_tree type = TREE_TYPE (fntype);
Christophe, thanks for your tests on glibc-2.29. With the above fix, I built glibc-2.29, and the previous error does not show up now. Could you please check if this fix works?
Do you have any suggestions on this fix?
All the best,
Peixin
-----Original Message-----
From: Richard Sandiford [mailto:richard.sandiford@arm.com]
Sent: Thursday, August 13, 2020 8:19 PM
To: Christophe Lyon <christophe.lyon@linaro.org>
Cc: qiaopeixin <qiaopeixin@huawei.com>; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Thu, 13 Aug 2020 at 03:54, qiaopeixin <qiaopeixin@huawei.com> wrote:
>>
>> Thanks for the review and commit.
>>
>> All the best,
>> Peixin
>>
>> -----Original Message-----
>> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
>> Sent: 2020年8月13日 0:25
>> To: qiaopeixin <qiaopeixin@huawei.com>
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH] AArch64: Add if condition in
>> aarch64_function_value [PR96479]
>>
>> qiaopeixin <qiaopeixin@huawei.com> writes:
>> > Hi,
>> >
>> > The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
>> >
>> > I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
>>
>> Thanks, pushed. I was initially sceptical because raising an error here and in aarch64_layout_arg is a hack. Both functions are just query functions and shouldn't have any side effects.
>>
>> The approach we took for FP modes seemed better: we define the FP move patterns unconditionally, and raise an error if we try to emit an FP move with !TARGET_FLOAT. This defers any error reporting until we actually try to generate code that depends on TARGET_FLOAT.
>>
>> But I guess SIMD stuff is different. There's no reason in principle why you can't use:
>>
>> unsigned short __attribute__((vector_size(8)))
>>
>> *within* a function with -mgeneral-regs-only. It would just need to be emulated, in the same way as for:
>>
>> unsigned short __attribute__((vector_size(4)))
>>
>> So it would be wrong to define the SIMD move patterns unconditionally and raise an error there.
>>
>> So all in all, I agree this is the best we can do given the current infrastructure.
>>
>
> Since this patch was committed my buildbot is broken for
> aarch64-linux-gnu because it now fails to build glibc-2.29:
> ../stdlib/bits/stdlib-float.h: In function 'atof':
> ../stdlib/bits/stdlib-float.h:26:1: error: '-mgeneral-regs-only' is
> incompatible with the use of floating-point types
Thanks for the heads-up. I've reverted the patch for now.
Looking more closely, it seems like aarch64_init_cumulative_args already tries to catch the problem that the patch was fixing:
if (!silent_p
&& !TARGET_FLOAT
&& fndecl && TREE_PUBLIC (fndecl)
&& fntype && fntype != error_mark_node)
{
const_tree type = TREE_TYPE (fntype);
machine_mode mode ATTRIBUTE_UNUSED; /* To pass pointer as argument. */
int nregs ATTRIBUTE_UNUSED; /* Likewise. */
if (aarch64_vfp_is_call_or_return_candidate (TYPE_MODE (type), type,
&mode, &nregs, NULL, false))
aarch64_err_no_fpadvsimd (TYPE_MODE (type));
}
The only reason it doesn't work for the testcase is that TREE_PUBLIC condition. TBH I'm not sure why it or the fndecl test is there:
this is just as problematic when calling via a function pointer or when calling a static function.
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-18 3:37 [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479] qiaopeixin
@ 2020-08-18 12:18 ` Christophe Lyon
2020-08-18 17:00 ` Richard Sandiford
1 sibling, 0 replies; 11+ messages in thread
From: Christophe Lyon @ 2020-08-18 12:18 UTC (permalink / raw)
To: qiaopeixin; +Cc: Richard Sandiford, gcc-patches
On Tue, 18 Aug 2020 at 05:38, qiaopeixin <qiaopeixin@huawei.com> wrote:
>
> Hi Richard,
>
> Thanks for the review and explanation.
>
> The previous fix adding if condition of TARGET_FLOAT does crash glibc-2.29.
>
> I checked the past log of writing the function aarch64_init_cumulative_args, and did not find the reason why Alan Lawrence added TREE_PUBLIC (fndecl) as one condition for entering the function type check. Maybe Alan could clarify? I tried to delete TREE_PUBLIC (fndecl), which turns out could solve both the glibc problem and the previous ICE problem. A new fix is made as following, passed bootstrap and deja test. I believe this fix is reasonable, since the function type should be checked no matter if it has external linkage or not.
>
> The function aarch64_init_cumulative_args checks the function types and should catch the error that "-mgeneral-regs-only" is incompatible with the use of SIMD/FP registers. In the test case on PR96479, the function myfunc2 returns one vector of 4 integers, while it is defined static type. TREE_PUBLIC (fndecl) is set as false and it prevents from entering if statement and checking function types. I delete "TREE_PUBLIC (fndecl)" so that gcc can catch the error through the function aarch64_init_cumulative_args now. The ICE on PR96479 can report the diagnostic error with this fix. The patch for the fix is attached as following:
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index b7f5bc76f1b..9ce83dce131 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -6017,7 +6017,7 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS *pcum,
>
> if (!silent_p
> && !TARGET_FLOAT
> - && fndecl && TREE_PUBLIC (fndecl)
> + && fndecl
> && fntype && fntype != error_mark_node)
> {
> const_tree type = TREE_TYPE (fntype);
>
> Christophe, thanks for your tests on glibc-2.29. With the above fix, I built glibc-2.29, and the previous error does not show up now. Could you please check if this fix works?
Hi,
I confirm this works OK for my testing (aarch64-linux-gnu and aarch64-elf)
Thanks,
Christophe
>
> Do you have any suggestions on this fix?
>
> All the best,
> Peixin
>
>
> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: Thursday, August 13, 2020 8:19 PM
> To: Christophe Lyon <christophe.lyon@linaro.org>
> Cc: qiaopeixin <qiaopeixin@huawei.com>; gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> > On Thu, 13 Aug 2020 at 03:54, qiaopeixin <qiaopeixin@huawei.com> wrote:
> >>
> >> Thanks for the review and commit.
> >>
> >> All the best,
> >> Peixin
> >>
> >> -----Original Message-----
> >> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> >> Sent: 2020年8月13日 0:25
> >> To: qiaopeixin <qiaopeixin@huawei.com>
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH] AArch64: Add if condition in
> >> aarch64_function_value [PR96479]
> >>
> >> qiaopeixin <qiaopeixin@huawei.com> writes:
> >> > Hi,
> >> >
> >> > The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
> >> >
> >> > I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
> >>
> >> Thanks, pushed. I was initially sceptical because raising an error here and in aarch64_layout_arg is a hack. Both functions are just query functions and shouldn't have any side effects.
> >>
> >> The approach we took for FP modes seemed better: we define the FP move patterns unconditionally, and raise an error if we try to emit an FP move with !TARGET_FLOAT. This defers any error reporting until we actually try to generate code that depends on TARGET_FLOAT.
> >>
> >> But I guess SIMD stuff is different. There's no reason in principle why you can't use:
> >>
> >> unsigned short __attribute__((vector_size(8)))
> >>
> >> *within* a function with -mgeneral-regs-only. It would just need to be emulated, in the same way as for:
> >>
> >> unsigned short __attribute__((vector_size(4)))
> >>
> >> So it would be wrong to define the SIMD move patterns unconditionally and raise an error there.
> >>
> >> So all in all, I agree this is the best we can do given the current infrastructure.
> >>
> >
> > Since this patch was committed my buildbot is broken for
> > aarch64-linux-gnu because it now fails to build glibc-2.29:
> > ../stdlib/bits/stdlib-float.h: In function 'atof':
> > ../stdlib/bits/stdlib-float.h:26:1: error: '-mgeneral-regs-only' is
> > incompatible with the use of floating-point types
>
> Thanks for the heads-up. I've reverted the patch for now.
>
> Looking more closely, it seems like aarch64_init_cumulative_args already tries to catch the problem that the patch was fixing:
>
> if (!silent_p
> && !TARGET_FLOAT
> && fndecl && TREE_PUBLIC (fndecl)
> && fntype && fntype != error_mark_node)
> {
> const_tree type = TREE_TYPE (fntype);
> machine_mode mode ATTRIBUTE_UNUSED; /* To pass pointer as argument. */
> int nregs ATTRIBUTE_UNUSED; /* Likewise. */
> if (aarch64_vfp_is_call_or_return_candidate (TYPE_MODE (type), type,
> &mode, &nregs, NULL, false))
> aarch64_err_no_fpadvsimd (TYPE_MODE (type));
> }
>
> The only reason it doesn't work for the testcase is that TREE_PUBLIC condition. TBH I'm not sure why it or the fndecl test is there:
> this is just as problematic when calling via a function pointer or when calling a static function.
>
> Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-18 3:37 [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479] qiaopeixin
2020-08-18 12:18 ` Christophe Lyon
@ 2020-08-18 17:00 ` Richard Sandiford
2020-08-19 9:33 ` qiaopeixin
1 sibling, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2020-08-18 17:00 UTC (permalink / raw)
To: qiaopeixin; +Cc: Christophe Lyon, gcc-patches
qiaopeixin <qiaopeixin@huawei.com> writes:
> Hi Richard,
>
> Thanks for the review and explanation.
>
> The previous fix adding if condition of TARGET_FLOAT does crash glibc-2.29.
>
> I checked the past log of writing the function aarch64_init_cumulative_args, and did not find the reason why Alan Lawrence added TREE_PUBLIC (fndecl) as one condition for entering the function type check. Maybe Alan could clarify? I tried to delete TREE_PUBLIC (fndecl), which turns out could solve both the glibc problem and the previous ICE problem. A new fix is made as following, passed bootstrap and deja test. I believe this fix is reasonable, since the function type should be checked no matter if it has external linkage or not.
>
> The function aarch64_init_cumulative_args checks the function types and should catch the error that "-mgeneral-regs-only" is incompatible with the use of SIMD/FP registers. In the test case on PR96479, the function myfunc2 returns one vector of 4 integers, while it is defined static type. TREE_PUBLIC (fndecl) is set as false and it prevents from entering if statement and checking function types. I delete "TREE_PUBLIC (fndecl)" so that gcc can catch the error through the function aarch64_init_cumulative_args now. The ICE on PR96479 can report the diagnostic error with this fix. The patch for the fix is attached as following:
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index b7f5bc76f1b..9ce83dce131 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -6017,7 +6017,7 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS *pcum,
>
> if (!silent_p
> && !TARGET_FLOAT
> - && fndecl && TREE_PUBLIC (fndecl)
> + && fndecl
> && fntype && fntype != error_mark_node)
> {
> const_tree type = TREE_TYPE (fntype);
I think the fndecl test is problematic too though. E.g. consider:
typedef int v4si __attribute__((vector_size(16)));
v4si (*foo) ();
void f (v4si *ptr) { *ptr = foo (); }
which ICEs for me even with the above.
I suggest we just remove the line and see whether anything breaks.
Thanks,
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-18 17:00 ` Richard Sandiford
@ 2020-08-19 9:33 ` qiaopeixin
2020-08-19 9:55 ` Richard Sandiford
0 siblings, 1 reply; 11+ messages in thread
From: qiaopeixin @ 2020-08-19 9:33 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Christophe Lyon, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]
Hi Richard,
Thanks for the example.
I remove the whole line "&&fndecl && TREE_PUBLIC (fndecl)" and passed bootstrap and deja tests. I add your provided example under /gcc.target/aarch64, and the patch is attached.
By the way, the new example also passed deja tests.
All the best,
Peixin
-----Original Message-----
From: Richard Sandiford [mailto:richard.sandiford@arm.com]
Sent: Wednesday, August 19, 2020 1:01 AM
To: qiaopeixin <qiaopeixin@huawei.com>
Cc: Christophe Lyon <christophe.lyon@linaro.org>; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
qiaopeixin <qiaopeixin@huawei.com> writes:
> Hi Richard,
>
> Thanks for the review and explanation.
>
> The previous fix adding if condition of TARGET_FLOAT does crash glibc-2.29.
>
> I checked the past log of writing the function aarch64_init_cumulative_args, and did not find the reason why Alan Lawrence added TREE_PUBLIC (fndecl) as one condition for entering the function type check. Maybe Alan could clarify? I tried to delete TREE_PUBLIC (fndecl), which turns out could solve both the glibc problem and the previous ICE problem. A new fix is made as following, passed bootstrap and deja test. I believe this fix is reasonable, since the function type should be checked no matter if it has external linkage or not.
>
> The function aarch64_init_cumulative_args checks the function types and should catch the error that "-mgeneral-regs-only" is incompatible with the use of SIMD/FP registers. In the test case on PR96479, the function myfunc2 returns one vector of 4 integers, while it is defined static type. TREE_PUBLIC (fndecl) is set as false and it prevents from entering if statement and checking function types. I delete "TREE_PUBLIC (fndecl)" so that gcc can catch the error through the function aarch64_init_cumulative_args now. The ICE on PR96479 can report the diagnostic error with this fix. The patch for the fix is attached as following:
>
> diff --git a/gcc/config/aarch64/aarch64.c
> b/gcc/config/aarch64/aarch64.c index b7f5bc76f1b..9ce83dce131 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -6017,7 +6017,7 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS
> *pcum,
>
> if (!silent_p
> && !TARGET_FLOAT
> - && fndecl && TREE_PUBLIC (fndecl)
> + && fndecl
> && fntype && fntype != error_mark_node)
> {
> const_tree type = TREE_TYPE (fntype);
I think the fndecl test is problematic too though. E.g. consider:
typedef int v4si __attribute__((vector_size(16)));
v4si (*foo) ();
void f (v4si *ptr) { *ptr = foo (); }
which ICEs for me even with the above.
I suggest we just remove the line and see whether anything breaks.
Thanks,
Richard
[-- Attachment #2: 0001-AArch64-Remove-fndecl-TREE_PUBLIC-fndecl-in-aarch64_.patch --]
[-- Type: application/octet-stream, Size: 1974 bytes --]
From f5e0956b409f45bb80d9e375da7d0ed7ac275ce7 Mon Sep 17 00:00:00 2001
From: Peixin Qiao <qiaopeixin@huawei.com>
Date: Thu, 20 Aug 2020 01:05:48 +0800
Subject: [PATCH] AArch64: Remove "fndecl && TREE_PUBLIC (fndecl)" in
aarch64_init_cumulative_args
This check will prevent the function type check of static funtion or calling
via a funtion pointer. The function type should be checked no matter if the
function has external linkage.
gcc/ChangeLog:
* config/aarch64/aarch64.c (aarch64_init_cumulative_args): Remove
"fndecl && TREE_PUBLIC (fndecl)" check since it prevents the funtion
type check when calling via a function pointer or when calling a static
function.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/mgeneral-regs_7.c: New test.
---
gcc/config/aarch64/aarch64.c | 1 -
gcc/testsuite/gcc.target/aarch64/mgeneral-regs_7.c | 11 +++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/mgeneral-regs_7.c
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index b7f5bc76f1b..b6d74496cd0 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -6017,7 +6017,6 @@ aarch64_init_cumulative_args (CUMULATIVE_ARGS *pcum,
if (!silent_p
&& !TARGET_FLOAT
- && fndecl && TREE_PUBLIC (fndecl)
&& fntype && fntype != error_mark_node)
{
const_tree type = TREE_TYPE (fntype);
diff --git a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_7.c b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_7.c
new file mode 100644
index 00000000000..3998947987c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_7.c
@@ -0,0 +1,11 @@
+/* { dg-options "-mgeneral-regs-only" } */
+
+typedef int v4si __attribute__ ((__vector_size__ ((16))));
+
+v4si (*foo) ();
+
+void
+f0 (v4si *ptr)
+{
+ *ptr = foo (); /* { dg-error "'-mgeneral-regs-only' is incompatible with the use of vector types" } */
+}
--
2.19.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-19 9:33 ` qiaopeixin
@ 2020-08-19 9:55 ` Richard Sandiford
2020-08-19 10:41 ` qiaopeixin
0 siblings, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2020-08-19 9:55 UTC (permalink / raw)
To: qiaopeixin; +Cc: Christophe Lyon, gcc-patches
qiaopeixin <qiaopeixin@huawei.com> writes:
> Hi Richard,
>
> Thanks for the example.
>
> I remove the whole line "&&fndecl && TREE_PUBLIC (fndecl)" and passed bootstrap and deja tests. I add your provided example under /gcc.target/aarch64, and the patch is attached.
Thanks, pushed to master.
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-19 9:55 ` Richard Sandiford
@ 2020-08-19 10:41 ` qiaopeixin
0 siblings, 0 replies; 11+ messages in thread
From: qiaopeixin @ 2020-08-19 10:41 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
Thanks.
All the best,
Peixin
-----Original Message-----
From: Richard Sandiford [mailto:richard.sandiford@arm.com]
Sent: Wednesday, August 19, 2020 5:56 PM
To: qiaopeixin <qiaopeixin@huawei.com>
Cc: Christophe Lyon <christophe.lyon@linaro.org>; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
qiaopeixin <qiaopeixin@huawei.com> writes:
> Hi Richard,
>
> Thanks for the example.
>
> I remove the whole line "&&fndecl && TREE_PUBLIC (fndecl)" and passed bootstrap and deja tests. I add your provided example under /gcc.target/aarch64, and the patch is attached.
Thanks, pushed to master.
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-13 11:43 ` Christophe Lyon
@ 2020-08-13 12:18 ` Richard Sandiford
0 siblings, 0 replies; 11+ messages in thread
From: Richard Sandiford @ 2020-08-13 12:18 UTC (permalink / raw)
To: Christophe Lyon; +Cc: qiaopeixin, gcc-patches
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Thu, 13 Aug 2020 at 03:54, qiaopeixin <qiaopeixin@huawei.com> wrote:
>>
>> Thanks for the review and commit.
>>
>> All the best,
>> Peixin
>>
>> -----Original Message-----
>> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
>> Sent: 2020年8月13日 0:25
>> To: qiaopeixin <qiaopeixin@huawei.com>
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
>>
>> qiaopeixin <qiaopeixin@huawei.com> writes:
>> > Hi,
>> >
>> > The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
>> >
>> > I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
>>
>> Thanks, pushed. I was initially sceptical because raising an error here and in aarch64_layout_arg is a hack. Both functions are just query functions and shouldn't have any side effects.
>>
>> The approach we took for FP modes seemed better: we define the FP move patterns unconditionally, and raise an error if we try to emit an FP move with !TARGET_FLOAT. This defers any error reporting until we actually try to generate code that depends on TARGET_FLOAT.
>>
>> But I guess SIMD stuff is different. There's no reason in principle why you can't use:
>>
>> unsigned short __attribute__((vector_size(8)))
>>
>> *within* a function with -mgeneral-regs-only. It would just need to be emulated, in the same way as for:
>>
>> unsigned short __attribute__((vector_size(4)))
>>
>> So it would be wrong to define the SIMD move patterns unconditionally and raise an error there.
>>
>> So all in all, I agree this is the best we can do given the current infrastructure.
>>
>
> Since this patch was committed my buildbot is broken for
> aarch64-linux-gnu because it now fails to build glibc-2.29:
> ../stdlib/bits/stdlib-float.h: In function 'atof':
> ../stdlib/bits/stdlib-float.h:26:1: error: '-mgeneral-regs-only' is
> incompatible with the use of floating-point types
Thanks for the heads-up. I've reverted the patch for now.
Looking more closely, it seems like aarch64_init_cumulative_args
already tries to catch the problem that the patch was fixing:
if (!silent_p
&& !TARGET_FLOAT
&& fndecl && TREE_PUBLIC (fndecl)
&& fntype && fntype != error_mark_node)
{
const_tree type = TREE_TYPE (fntype);
machine_mode mode ATTRIBUTE_UNUSED; /* To pass pointer as argument. */
int nregs ATTRIBUTE_UNUSED; /* Likewise. */
if (aarch64_vfp_is_call_or_return_candidate (TYPE_MODE (type), type,
&mode, &nregs, NULL, false))
aarch64_err_no_fpadvsimd (TYPE_MODE (type));
}
The only reason it doesn't work for the testcase is that TREE_PUBLIC
condition. TBH I'm not sure why it or the fndecl test is there:
this is just as problematic when calling via a function pointer
or when calling a static function.
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-13 1:54 ` qiaopeixin
@ 2020-08-13 11:43 ` Christophe Lyon
2020-08-13 12:18 ` Richard Sandiford
0 siblings, 1 reply; 11+ messages in thread
From: Christophe Lyon @ 2020-08-13 11:43 UTC (permalink / raw)
To: qiaopeixin; +Cc: Richard Sandiford, gcc-patches
Hi,
On Thu, 13 Aug 2020 at 03:54, qiaopeixin <qiaopeixin@huawei.com> wrote:
>
> Thanks for the review and commit.
>
> All the best,
> Peixin
>
> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: 2020年8月13日 0:25
> To: qiaopeixin <qiaopeixin@huawei.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
>
> qiaopeixin <qiaopeixin@huawei.com> writes:
> > Hi,
> >
> > The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
> >
> > I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
>
> Thanks, pushed. I was initially sceptical because raising an error here and in aarch64_layout_arg is a hack. Both functions are just query functions and shouldn't have any side effects.
>
> The approach we took for FP modes seemed better: we define the FP move patterns unconditionally, and raise an error if we try to emit an FP move with !TARGET_FLOAT. This defers any error reporting until we actually try to generate code that depends on TARGET_FLOAT.
>
> But I guess SIMD stuff is different. There's no reason in principle why you can't use:
>
> unsigned short __attribute__((vector_size(8)))
>
> *within* a function with -mgeneral-regs-only. It would just need to be emulated, in the same way as for:
>
> unsigned short __attribute__((vector_size(4)))
>
> So it would be wrong to define the SIMD move patterns unconditionally and raise an error there.
>
> So all in all, I agree this is the best we can do given the current infrastructure.
>
Since this patch was committed my buildbot is broken for
aarch64-linux-gnu because it now fails to build glibc-2.29:
../stdlib/bits/stdlib-float.h: In function 'atof':
../stdlib/bits/stdlib-float.h:26:1: error: '-mgeneral-regs-only' is
incompatible with the use of floating-point types
I haven't yet tried a more recent glibc version, not sure if 2.29 is
considered obsolete?
Christophe
> Thanks,
> Richard
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-12 16:25 ` Richard Sandiford
@ 2020-08-13 1:54 ` qiaopeixin
2020-08-13 11:43 ` Christophe Lyon
0 siblings, 1 reply; 11+ messages in thread
From: qiaopeixin @ 2020-08-13 1:54 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
Thanks for the review and commit.
All the best,
Peixin
-----Original Message-----
From: Richard Sandiford [mailto:richard.sandiford@arm.com]
Sent: 2020年8月13日 0:25
To: qiaopeixin <qiaopeixin@huawei.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
qiaopeixin <qiaopeixin@huawei.com> writes:
> Hi,
>
> The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
>
> I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
Thanks, pushed. I was initially sceptical because raising an error here and in aarch64_layout_arg is a hack. Both functions are just query functions and shouldn't have any side effects.
The approach we took for FP modes seemed better: we define the FP move patterns unconditionally, and raise an error if we try to emit an FP move with !TARGET_FLOAT. This defers any error reporting until we actually try to generate code that depends on TARGET_FLOAT.
But I guess SIMD stuff is different. There's no reason in principle why you can't use:
unsigned short __attribute__((vector_size(8)))
*within* a function with -mgeneral-regs-only. It would just need to be emulated, in the same way as for:
unsigned short __attribute__((vector_size(4)))
So it would be wrong to define the SIMD move patterns unconditionally and raise an error there.
So all in all, I agree this is the best we can do given the current infrastructure.
Thanks,
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
2020-08-06 13:01 qiaopeixin
@ 2020-08-12 16:25 ` Richard Sandiford
2020-08-13 1:54 ` qiaopeixin
0 siblings, 1 reply; 11+ messages in thread
From: Richard Sandiford @ 2020-08-12 16:25 UTC (permalink / raw)
To: qiaopeixin; +Cc: gcc-patches
qiaopeixin <qiaopeixin@huawei.com> writes:
> Hi,
>
> The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
>
> I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
Thanks, pushed. I was initially sceptical because raising an error here
and in aarch64_layout_arg is a hack. Both functions are just query
functions and shouldn't have any side effects.
The approach we took for FP modes seemed better: we define the FP move
patterns unconditionally, and raise an error if we try to emit an FP move
with !TARGET_FLOAT. This defers any error reporting until we actually
try to generate code that depends on TARGET_FLOAT.
But I guess SIMD stuff is different. There's no reason in principle why
you can't use:
unsigned short __attribute__((vector_size(8)))
*within* a function with -mgeneral-regs-only. It would just need to be
emulated, in the same way as for:
unsigned short __attribute__((vector_size(4)))
So it would be wrong to define the SIMD move patterns unconditionally
and raise an error there.
So all in all, I agree this is the best we can do given the current
infrastructure.
Thanks,
Richard
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
@ 2020-08-06 13:01 qiaopeixin
2020-08-12 16:25 ` Richard Sandiford
0 siblings, 1 reply; 11+ messages in thread
From: qiaopeixin @ 2020-08-06 13:01 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 417 bytes --]
Hi,
The test case vector-subscript-2.c in the gcc testsuit will report an ICE in the expand pass since '-mgeneral-regs-only' is incompatible with the use of V4SI mode. I propose to report the diagnostic information instead of ICE, and the problem has been discussed on PR 96479.
I attached the patch to solve the problem. Bootstrapped and tested on aarch64-linux-gnu. Any suggestions?
All the best,
Peixin
[-- Attachment #2: 0001-AArch64-Add-if-condition-in-aarch64_function_value-P.patch --]
[-- Type: application/octet-stream, Size: 2380 bytes --]
From 79b463a0296818a24974db27fcb1d826762e3192 Mon Sep 17 00:00:00 2001
From: Peixin Qiao <qiaopeixin@huawei.com>
Date: Thu, 6 Aug 2020 22:40:53 +0800
Subject: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]
Report diagnostic information instead of ICE if it generats fp/simd for
return register when fp/simd is disabled by -mgeneral-regs-only.
gcc/ChangeLog:
* config/aarch64/aarch64.c (aarch64_function_value): Add if
condition to check ag_mode after entering if condition of
aarch64_vfp_is_call_or_return_candidate. If TARGET_FLOAT is
set as false by -mgeneral-regs-only, report the diagnostic
information of -mgeneral-regs-only imcompatible with the use
of fp/simd register(s).
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/mgeneral-regs_1.c: Add the comment that
-mgeneral-regs-only is compatible with the use of vector type
used in the test case.
---
gcc/config/aarch64/aarch64.c | 4 ++++
gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c | 4 ++--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 2a0fd714e29..9c916b86431 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5517,6 +5517,10 @@ aarch64_function_value (const_tree type, const_tree func,
if (aarch64_vfp_is_call_or_return_candidate (mode, type, &ag_mode, &count,
NULL, false))
{
+ /* TYPE passed in fp/simd registers. */
+ if (!TARGET_FLOAT)
+ aarch64_err_no_fpadvsimd (ag_mode);
+
gcc_assert (!sve_p);
if (!aarch64_composite_type_p (type, mode))
{
diff --git a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c
index 336402ead84..6f785c99b74 100644
--- a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_1.c
@@ -4,6 +4,6 @@ typedef int int32x2_t __attribute__ ((__vector_size__ ((8))));
/* { dg-error "'-mgeneral-regs-only' is incompatible with the use of vector types" "" {target "aarch64*-*-*"} .+1 } */
int32x2_t test (int32x2_t a, int32x2_t b)
-{
- return a + b;
+{ /* { dg-error "'-mgeneral-regs-only' is incompatible with the use of vector types" } */
+ return a + b; /* { dg-error "'-mgeneral-regs-only' is incompatible with the use of vector types" } */
}
--
2.19.1
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-08-19 10:41 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-18 3:37 [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479] qiaopeixin
2020-08-18 12:18 ` Christophe Lyon
2020-08-18 17:00 ` Richard Sandiford
2020-08-19 9:33 ` qiaopeixin
2020-08-19 9:55 ` Richard Sandiford
2020-08-19 10:41 ` qiaopeixin
-- strict thread matches above, loose matches on Subject: below --
2020-08-06 13:01 qiaopeixin
2020-08-12 16:25 ` Richard Sandiford
2020-08-13 1:54 ` qiaopeixin
2020-08-13 11:43 ` Christophe Lyon
2020-08-13 12:18 ` Richard Sandiford
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).