public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* 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
* [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).