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

[qiaopeixin]

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.sandif...@arm.com] 
Sent: Wednesday, August 19, 2020 1:01 AM
To: qiaopeixin <qiaopei...@huawei.com>
Cc: Christophe Lyon <christophe.l...@linaro.org>; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] AArch64: Add if condition in aarch64_function_value 
[PR96479]

qiaopeixin <qiaopei...@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

0001-AArch64-Remove-fndecl-TREE_PUBLIC-fndecl-in-aarch64_.patch
Description: 0001-AArch64-Remove-fndecl-TREE_PUBLIC-fndecl-in-aarch64_.patch