Hi Bill,
on 2021/9/29 下午7:59, Bill Schmidt wrote:
> Hi Kewen,
>
> On 9/28/21 9:34 PM, Kewen.Lin wrote:
>> 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.
>
> OK. If you are comfortable that this will be caught when the builtin is
> actually not valid, then I'll
> withdraw my objection. Can you test it? I know that we've been trying to
> fix these cases piecemeal
> in the old support, and as Peter says it's important to backport this, we
> need the solution. I just
> want to be sure we're not breaking something, and test coverage in this area
> is pretty terrible.
>
Thanks for the comments and the trust! I found I missed to type the function
name
rs6000_expand_builtin for expanding part, specifically the function has:
...
bool func_valid_p = ((rs6000_builtin_mask & mask) == mask);
...
if (!func_valid_p)
{
rs6000_invalid_builtin (fcode); // It emits error here.
/* Given it is invalid, just generate a normal call. */
return expand_call (exp, target, ignore);
}
IIUC, all invalid built-ins will eventually be caught by this function (as
mentioned
before, the built-in gimple folding would bypass the invalid built-ins).
I tested the below case:
#ifndef EXPECT_ERROR
#pragma GCC target "cpu=power10"
#endif
int main() {
float *b;
__vector_quad c;
__builtin_mma_disassemble_acc(b, &c);
return 0;
}
Option set 1 (S1): -mcpu=power9 -c
Option set 2 (S2): -mcpu=power9 -c -DEXPECT_ERROR
Option set 3 (S3): -mcpu=power9 -c -flto
Option set 4 (S4): -mcpu=power9 -c -flto -DEXPECT_ERROR
Option set 5 (S5): -mcpu=power9 -flto (lto linking)
Option set 6 (S6): -mcpu=power9 -flto -DEXPECT_ERROR (lto linking)
Option set 7 (S7): -mcpu=power9 -c -flto -ffat-lto-objects
Option set 8 (S8): -mcpu=power9 -c -flto -ffat-lto-objects -DEXPECT_ERROR
w/o fix w/ fix
S1 PASS PASS
S2 ERROR ERROR
S3 PASS PASS
S4 PASS PASS
S5 ERROR PASS
S6 ERROR ERROR
S7 PASS PASS
S8 ERROR ERROR
As above, this patch fixes the unexpected error in S5 and keeps the other
PASS/ERROR
as the original. Note that S4 PASS is expected since expansion isn't needed
when
generating non-fat lto objects, the error happens during linking (S6).
Based on the understanding and testing, I think it's safe to adopt this patch.
Do both Peter and you agree the rs6000_expand_builtin will catch the invalid
built-in?
Is there some special case which probably escapes out?
By the way, I tested the bif rewriting patch series V5, it couldn't make the
original
case in PR (S5) pass, I may miss something or the used series isn't up-to-date.
Could
you help to have a try? I agree with Peter, if the rewriting can fix this
issue, then
we don't need this patch for trunk any more, I'm happy to abandon this. :)
BR,
Kewen
> Thanks!
> Bill
>
>>
>>> 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
>>>>
>>
>
