Hi Bill,
Thanks for your prompt comments!
on 2021/9/29 上午3:24, Bill Schmidt wrote:
> Hi Kewen,
>
> Although I agree that what we do now is tragically bad (and will be fixed in
> the builtin rewrite), this seems a little too cavalier to remove all checking
> during initialization without adding any checking somewhere else. :-) We
> still need to check for invalid usage when the builtin is expanded, and I
> don't think the old code does this at all.
>
If I read the code right, there are some following places to check the invalid
usage or not.
1) for folding, rs6000_gimple_fold_builtin -> rs6000_builtin_is_supported_p
-> check mask
-> defer to expand if invalid.
2) for expanding, obtain func_valid_p, error in rs6000_invalid_builtin.
Both places seem to exist before the builtin rewrite, am I missing something?
btw, I remembered I used one built gcc with my fix to compile one test case
which is supposed to fail
due to its invalid usage builtin at option -flto, it failed (errored) as
expected but at LTRANS phase
since it's the time to do expansion for no-fat-objs scenario.
> Unless you are planning to do a backport, I think the proper way forward here
> is to just wait for the new builtin support to land. In the new code, we
> initialize all built-ins up front, and check properly at expansion time
> whether the builtin is enabled in the environment that obtains during expand.
Good to know that! Nice! btw, for this issue itself, the current
implementation (without rewriting)
also initializes the built-ins in the table since MMA built-ins guarded in
TARGET_EXTRA_BUILTINS,
the root cause is the rs6000_builtin_mask can't set up (be switched) expectedly
since the checking
time is too early right when the built-in function_decl being created.
BR,
Kewen
>
> My two cents,
> Bill
>
> On 9/28/21 3:13 AM, Kewen.Lin wrote:
>> Hi,
>>
>> As the discussion in PR102347, currently builtin_decl is invoked so
>> early, it's when making up the function_decl for builtin functions,
>> at that time the rs6000_builtin_mask could be wrong for those
>> builtins sitting in #pragma/attribute target functions, though it
>> will be updated properly later when LTO processes all nodes.
>>
>> This patch is to align with the practice i386 port adopts, also
>> align with r10-7462 by relaxing builtin mask checking in some places.
>>
>> Bootstrapped and regress-tested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> -----
>> gcc/ChangeLog:
>>
>> PR target/102347
>> * config/rs6000/rs6000-call.c (rs6000_builtin_decl): Remove builtin
>> mask check.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR target/102347
>> * gcc.target/powerpc/pr102347.c: New test.
>>
>> ---
>> gcc/config/rs6000/rs6000-call.c | 14 ++++----------
>> gcc/testsuite/gcc.target/powerpc/pr102347.c | 15 +++++++++++++++
>> 2 files changed, 19 insertions(+), 10 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr102347.c
>>
>> diff --git a/gcc/config/rs6000/rs6000-call.c
>> b/gcc/config/rs6000/rs6000-call.c
>> index fd7f24da818..15e0e09c07d 100644
>> --- a/gcc/config/rs6000/rs6000-call.c
>> +++ b/gcc/config/rs6000/rs6000-call.c
>> @@ -13775,23 +13775,17 @@ rs6000_init_builtins (void)
>> }
>> }
>>
>> -/* Returns the rs6000 builtin decl for CODE. */
>> +/* Returns the rs6000 builtin decl for CODE. Note that we don't check
>> + the builtin mask here since there could be some #pragma/attribute
>> + target functions and the rs6000_builtin_mask could be wrong when
>> + this checking happens, though it will be updated properly later. */
>>
>> tree
>> rs6000_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED)
>> {
>> - HOST_WIDE_INT fnmask;
>> -
>> if (code >= RS6000_BUILTIN_COUNT)
>> return error_mark_node;
>>
>> - fnmask = rs6000_builtin_info[code].mask;
>> - if ((fnmask & rs6000_builtin_mask) != fnmask)
>> - {
>> - rs6000_invalid_builtin ((enum rs6000_builtins)code);
>> - return error_mark_node;
>> - }
>> -
>> return rs6000_builtin_decls[code];
>> }
>>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr102347.c
>> b/gcc/testsuite/gcc.target/powerpc/pr102347.c
>> new file mode 100644
>> index 00000000000..05c439a8dac
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr102347.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-do link } */
>> +/* { dg-require-effective-target power10_ok } */
>> +/* { dg-require-effective-target lto } */
>> +/* { dg-options "-flto -mdejagnu-cpu=power9" } */
>> +
>> +/* Verify there are no error messages in LTO mode. */
>> +
>> +#pragma GCC target "cpu=power10"
>> +int main ()
>> +{
>> + float *b;
>> + __vector_quad c;
>> + __builtin_mma_disassemble_acc (b, &c);
>> + return 0;
>> +}
>> --
>> 2.27.0
>>
>
