[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.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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AArch64-Remove-fndecl-TREE_PUBLIC-fndecl-in-aarch64_.patch
Type: application/octet-stream
Size: 1974 bytes
Desc: 0001-AArch64-Remove-fndecl-TREE_PUBLIC-fndecl-in-aarch64_.patch
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200819/0dbd72aa/attachment.obj>