[PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[qiaopeixin]

[-- 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

Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[Richard Sandiford]

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

RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[qiaopeixin]

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

Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[Christophe Lyon]

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
>

Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[Richard Sandiford]

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

RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[qiaopeixin]

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

Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[Richard Sandiford]

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

RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[qiaopeixin]

[-- 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

Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[Richard Sandiford]

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

Re: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[Christophe Lyon]

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

RE: [PATCH] AArch64: Add if condition in aarch64_function_value [PR96479]

[qiaopeixin]

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