| Message ID | 7505a666-7b51-255c-9908-aabc753f7c33@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Remove builtin mask check from builtin_decl [PR102347] | expand |
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. 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. 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 >
On 9/28/21 2:24 PM, Bill Schmidt via Gcc-patches wrote: > 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. Bill, have you tried the test case in the bugzilla with your builtin rewrite and it works? If so, then I think the correct thing would be to skip "fixing" trunk and wait for your builtin rewrite to land there. That said, this does fail on GCC11 and GCC10 (not sure about earlier, haven't tried them yet), so we will need some type of fix for the releases. I do think your concern about not having some checking elsewhere is valid, unless there already is some checking there and you and I are just not aware of it. Peter
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 >> >
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! 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 >>> >
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 >>>> >> >
Hi! On Thu, Sep 30, 2021 at 11:06:50AM +0800, Kewen.Lin wrote: [ huge snip ] > 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? The function rs6000_builtin_decl has a terribly generic name. Where all is it called from? Do all such places allow the change in semantics? Do any comments or other documentation need to change? Is the function name still good? > 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. :) (Mail lines are 70 or so chars max, so that they can be quoted a few levels). If we do need a band-aid for 10 and 11 (and we do as far as I can see), I'd like to see one for just MMA there, and let all other badness fade into history. Unless you can convince me (in the patch / commit message) that this is safe :-) Whichever way you choose, it is likely best to do the same on 10 and 11 as on trunk, since it will all be replaced on trunk soon anyway. Segher
Hi Segher, Thanks for the comments. on 2021/10/1 上午6:13, Segher Boessenkool wrote: > Hi! > > On Thu, Sep 30, 2021 at 11:06:50AM +0800, Kewen.Lin wrote: > > [ huge snip ] > >> 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? > > The function rs6000_builtin_decl has a terribly generic name. Where all > is it called from? Do all such places allow the change in semantics? > Do any comments or other documentation need to change? Is the function > name still good? % grep -rE "\<builtin_decl\> \(" . ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); ./gcc/config/aarch64/aarch64.c: return aarch64_sve::builtin_decl (subcode, initialize_p); ./gcc/config/aarch64/aarch64-protos.h: tree builtin_decl (unsigned, bool); ./gcc/config/aarch64/aarch64-sve-builtins.cc:builtin_decl (unsigned int code, bool) ./gcc/tree-streamer-in.c: tree result = targetm.builtin_decl (fcode, true); % grep -rE "\<rs6000_builtin_decl\> \(" . ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node ./gcc/config/rs6000/rs6000-gen-builtins.c: "extern tree rs6000_builtin_decl (unsigned, " ./gcc/config/rs6000/rs6000-call.c:rs6000_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED) ./gcc/config/rs6000/rs6000-internal.h:extern tree rs6000_builtin_decl (unsigned code, As above, the call sites are mainly in 1) function unpack_ts_function_decl_value_fields in gcc/tree-streamer-in.c 2) function altivec_resolve_new_overloaded_builtin in gcc/config/rs6000/rs6000-c.c 2) is newly introduced by Bill's bif rewriting patch series, all uses in it are along with rs6000_new_builtin_is_supported which adopts a new way to check bif supported or not (the old rs6000_builtin_is_supported_p uses builtin mask), so I think the builtin mask checking is useless (unexpected?) for these uses. Besides, the description for this hook: "tree TARGET_BUILTIN_DECL (unsigned code, bool initialize_p) [Target Hook] Define this hook if you have any machine-specific built-in functions that need to be defined. It should be a function that returns the builtin function declaration for the builtin function code code. If there is no such builtin and it cannot be initialized at this time if initialize p is true the function should return NULL_TREE. If code is out of range the function should return error_mark_node." It would only return error_mark_node when the code is out of range. The current rs6000_builtin_decl returns error_mark_node not only for "out of range", it looks inconsistent and this patch also revise it. The hook was introduced by commit e9e4b3a892d0d19418f23bb17bdeac33f9a8bfd2, it meant to ensure the bif function_decl is valid (check if bif code in the range and the corresponding entry in bif table is not NULL). May be better with name check_and_get_builtin_decl? CC Richi, he may have more insights. > >> 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. :) > > (Mail lines are 70 or so chars max, so that they can be quoted a few > levels). > ah, OK, thanks. :) > If we do need a band-aid for 10 and 11 (and we do as far as I can see), > I'd like to see one for just MMA there, and let all other badness fade > into history. Unless you can convince me (in the patch / commit > message) that this is safe :-) Just to fix for MMA seems incomplete to me since we can simply construct one non-MMA but failed case. I questioned in the other thread, is there any possibility for one invalid target specific bif to escape from the function rs6000_expand_builtin? (note that folding won't handle invalid bifs, so invalid bifs won't get folded early.) If no, I think it would be safe. > > Whichever way you choose, it is likely best to do the same on 10 and 11 > as on trunk, since it will all be replaced on trunk soon anyway. > OK, will see Bill's reply (he should be back from vacation soon). :) BR, Kewen
Hi Kewen, On 10/11/21 1:30 AM, Kewen.Lin wrote: > Hi Segher, > > Thanks for the comments. > > on 2021/10/1 上午6:13, Segher Boessenkool wrote: >> Hi! >> >> On Thu, Sep 30, 2021 at 11:06:50AM +0800, Kewen.Lin wrote: >> >> [ huge snip ] >> >>> 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? >> The function rs6000_builtin_decl has a terribly generic name. Where all >> is it called from? Do all such places allow the change in semantics? >> Do any comments or other documentation need to change? Is the function >> name still good? > > % grep -rE "\<builtin_decl\> \(" . > ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); > ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); > ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); > ./gcc/config/aarch64/aarch64.c: return aarch64_sve::builtin_decl (subcode, initialize_p); > ./gcc/config/aarch64/aarch64-protos.h: tree builtin_decl (unsigned, bool); > ./gcc/config/aarch64/aarch64-sve-builtins.cc:builtin_decl (unsigned int code, bool) > ./gcc/tree-streamer-in.c: tree result = targetm.builtin_decl (fcode, true); > > % grep -rE "\<rs6000_builtin_decl\> \(" . > ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node > ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node > ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node > ./gcc/config/rs6000/rs6000-gen-builtins.c: "extern tree rs6000_builtin_decl (unsigned, " > ./gcc/config/rs6000/rs6000-call.c:rs6000_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED) > ./gcc/config/rs6000/rs6000-internal.h:extern tree rs6000_builtin_decl (unsigned code, > > As above, the call sites are mainly in > 1) function unpack_ts_function_decl_value_fields in gcc/tree-streamer-in.c > 2) function altivec_resolve_new_overloaded_builtin in gcc/config/rs6000/rs6000-c.c > > 2) is newly introduced by Bill's bif rewriting patch series, all uses in it are > along with rs6000_new_builtin_is_supported which adopts a new way to check bif > supported or not (the old rs6000_builtin_is_supported_p uses builtin mask), so > I think the builtin mask checking is useless (unexpected?) for these uses. Things are a bit confused because we are part way through the patch series. rs6000_builtin_decl will be changed to redirect to rs6000_new_builtin_decl when using the new builtin support. That function will be: static tree rs6000_new_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED) { rs6000_gen_builtins fcode = (rs6000_gen_builtins) code; if (fcode >= RS6000_OVLD_MAX) return error_mark_node; if (!rs6000_new_builtin_is_supported (fcode)) { rs6000_invalid_new_builtin (fcode); return error_mark_node; } return rs6000_builtin_decls_x[code]; } So, as you surmise, this will be using the new method of testing for builtin validity. You can ignore the rs6000-c.c and rs6000-gen-builtins.c references of rs6000_builtin_decl for purposes of fixing the existing way of doing things. > > Besides, the description for this hook: > > "tree TARGET_BUILTIN_DECL (unsigned code, bool initialize_p) [Target Hook] > Define this hook if you have any machine-specific built-in functions that need to be > defined. It should be a function that returns the builtin function declaration for the > builtin function code code. If there is no such builtin and it cannot be initialized at > this time if initialize p is true the function should return NULL_TREE. If code is out > of range the function should return error_mark_node." > > It would only return error_mark_node when the code is out of range. The current > rs6000_builtin_decl returns error_mark_node not only for "out of range", it looks > inconsistent and this patch also revise it. > > The hook was introduced by commit e9e4b3a892d0d19418f23bb17bdeac33f9a8bfd2, > it meant to ensure the bif function_decl is valid (check if bif code in the > range and the corresponding entry in bif table is not NULL). May be better > with name check_and_get_builtin_decl? CC Richi, he may have more insights. > >>> 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. :) >> (Mail lines are 70 or so chars max, so that they can be quoted a few >> levels). >> > ah, OK, thanks. :) > >> If we do need a band-aid for 10 and 11 (and we do as far as I can see), >> I'd like to see one for just MMA there, and let all other badness fade >> into history. Unless you can convince me (in the patch / commit >> message) that this is safe :-) > Just to fix for MMA seems incomplete to me since we can simply > construct one non-MMA but failed case. I questioned in the other > thread, is there any possibility for one invalid target specific > bif to escape from the function rs6000_expand_builtin? (note that > folding won't handle invalid bifs, so invalid bifs won't get folded > early.) If no, I think it would be safe. Yes, looking at what you wrote in the other thread about your investigations with -DEXPECT_ERROR, the existing rs6000_expand_builtin logic makes the patch safe from my perspective. Thank you for digging in and checking! > >> Whichever way you choose, it is likely best to do the same on 10 and 11 >> as on trunk, since it will all be replaced on trunk soon anyway. >> > OK, will see Bill's reply (he should be back from vacation soon). :) I am back! :) Agree with Segher that backporting this after a sensible interval would be appropriate. We have been fixing these things piecemeal for too long, so if we have a solid general fix, I'm a fan. Going forward with 12 and later we can rip off the band-aid and do things right. Thanks! Bill > > BR, > Kewen
Hi Bill! on 2021/10/13 上午12:36, Bill Schmidt wrote: > Hi Kewen, > > On 10/11/21 1:30 AM, Kewen.Lin wrote: >> Hi Segher, >> >> Thanks for the comments. >> >> on 2021/10/1 上午6:13, Segher Boessenkool wrote: >>> Hi! >>> >>> On Thu, Sep 30, 2021 at 11:06:50AM +0800, Kewen.Lin wrote: >>> >>> [ huge snip ] >>> >>>> 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? >>> The function rs6000_builtin_decl has a terribly generic name. Where all >>> is it called from? Do all such places allow the change in semantics? >>> Do any comments or other documentation need to change? Is the function >>> name still good? >> >> % grep -rE "\<builtin_decl\> \(" . >> ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); >> ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); >> ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true); >> ./gcc/config/aarch64/aarch64.c: return aarch64_sve::builtin_decl (subcode, initialize_p); >> ./gcc/config/aarch64/aarch64-protos.h: tree builtin_decl (unsigned, bool); >> ./gcc/config/aarch64/aarch64-sve-builtins.cc:builtin_decl (unsigned int code, bool) >> ./gcc/tree-streamer-in.c: tree result = targetm.builtin_decl (fcode, true); >> >> % grep -rE "\<rs6000_builtin_decl\> \(" . >> ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node >> ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node >> ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl (instance->bifid, false) != error_mark_node >> ./gcc/config/rs6000/rs6000-gen-builtins.c: "extern tree rs6000_builtin_decl (unsigned, " >> ./gcc/config/rs6000/rs6000-call.c:rs6000_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED) >> ./gcc/config/rs6000/rs6000-internal.h:extern tree rs6000_builtin_decl (unsigned code, >> >> As above, the call sites are mainly in >> 1) function unpack_ts_function_decl_value_fields in gcc/tree-streamer-in.c >> 2) function altivec_resolve_new_overloaded_builtin in gcc/config/rs6000/rs6000-c.c >> >> 2) is newly introduced by Bill's bif rewriting patch series, all uses in it are >> along with rs6000_new_builtin_is_supported which adopts a new way to check bif >> supported or not (the old rs6000_builtin_is_supported_p uses builtin mask), so >> I think the builtin mask checking is useless (unexpected?) for these uses. > > Things are a bit confused because we are part way through the patch series. > rs6000_builtin_decl will be changed to redirect to rs6000_new_builtin_decl when > using the new builtin support. That function will be: > > static tree > rs6000_new_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED) > { > rs6000_gen_builtins fcode = (rs6000_gen_builtins) code; > > if (fcode >= RS6000_OVLD_MAX) > return error_mark_node; > > if (!rs6000_new_builtin_is_supported (fcode)) > { > rs6000_invalid_new_builtin (fcode); > return error_mark_node; > } > > return rs6000_builtin_decls_x[code]; > } > > So, as you surmise, this will be using the new method of testing for builtin validity. > You can ignore the rs6000-c.c and rs6000-gen-builtins.c references of rs6000_builtin_decl > for purposes of fixing the existing way of doing things. > Thanks for the explanation, it makes more sense. >> >> Besides, the description for this hook: >> >> "tree TARGET_BUILTIN_DECL (unsigned code, bool initialize_p) [Target Hook] >> Define this hook if you have any machine-specific built-in functions that need to be >> defined. It should be a function that returns the builtin function declaration for the >> builtin function code code. If there is no such builtin and it cannot be initialized at >> this time if initialize p is true the function should return NULL_TREE. If code is out >> of range the function should return error_mark_node." >> >> It would only return error_mark_node when the code is out of range. The current >> rs6000_builtin_decl returns error_mark_node not only for "out of range", it looks >> inconsistent and this patch also revise it. >> >> The hook was introduced by commit e9e4b3a892d0d19418f23bb17bdeac33f9a8bfd2, >> it meant to ensure the bif function_decl is valid (check if bif code in the >> range and the corresponding entry in bif table is not NULL). May be better >> with name check_and_get_builtin_decl? CC Richi, he may have more insights. >> >>>> 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. :) >>> (Mail lines are 70 or so chars max, so that they can be quoted a few >>> levels). >>> >> ah, OK, thanks. :) >> >>> If we do need a band-aid for 10 and 11 (and we do as far as I can see), >>> I'd like to see one for just MMA there, and let all other badness fade >>> into history. Unless you can convince me (in the patch / commit >>> message) that this is safe :-) >> Just to fix for MMA seems incomplete to me since we can simply >> construct one non-MMA but failed case. I questioned in the other >> thread, is there any possibility for one invalid target specific >> bif to escape from the function rs6000_expand_builtin? (note that >> folding won't handle invalid bifs, so invalid bifs won't get folded >> early.) If no, I think it would be safe. > > Yes, looking at what you wrote in the other thread about your investigations > with -DEXPECT_ERROR, the existing rs6000_expand_builtin logic makes the > patch safe from my perspective. Thank you for digging in and checking! > Thanks for confirming! Segher, we think this patch is safe and it also makes the function behave more consistently as the description of the hook. Does this patch look good to you? >> >>> Whichever way you choose, it is likely best to do the same on 10 and 11 >>> as on trunk, since it will all be replaced on trunk soon anyway. >>> >> OK, will see Bill's reply (he should be back from vacation soon). :) > > I am back! :) Agree with Segher that backporting this after a sensible > interval would be appropriate. We have been fixing these things > piecemeal for too long, so if we have a solid general fix, I'm a fan. > Going forward with 12 and later we can rip off the band-aid and do things > right. Got it, thanks! BR, Kewen
Hi, As the discussions and the testing result under the main thread, this patch would be safe. Ping for this: https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580357.html BR, Kewen on 2021/9/28 下午4:13, Kewen.Lin via Gcc-patches 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 >
Hi, As the discussions and the testing result under the main thread, this patch would be safe. Ping for this: https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580357.html BR, Kewen > > on 2021/9/28 下午4:13, Kewen.Lin via Gcc-patches 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 >> >
Hi, As the discussions and the testing result under the main thread, this patch would be safe. Ping for this: https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580357.html BR, Kewen >> on 2021/9/28 下午4:13, Kewen.Lin via Gcc-patches 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 >>> >>
Hi! On Mon, Oct 11, 2021 at 02:30:42PM +0800, Kewen.Lin wrote: > > If we do need a band-aid for 10 and 11 (and we do as far as I can see), > > I'd like to see one for just MMA there, and let all other badness fade > > into history. Unless you can convince me (in the patch / commit > > message) that this is safe :-) > > Just to fix for MMA seems incomplete to me since we can simply > construct one non-MMA but failed case. I questioned in the other > thread, is there any possibility for one invalid target specific > bif to escape from the function rs6000_expand_builtin? (note that > folding won't handle invalid bifs, so invalid bifs won't get folded > early.) If no, I think it would be safe. It is much safer and simpler to not backport anything we do not need to. You need to come up with a reason why it would be a good idea to do backport something, especially if it isn't obviously super safe. Segher
Hi! On Tue, Sep 28, 2021 at 04:13:40PM +0800, Kewen.Lin wrote: > PR target/102347 > * config/rs6000/rs6000-call.c (rs6000_builtin_decl): Remove builtin > mask check. (Don't wrap lines early please). Okay for trunk and all backports. Thanks! Segher
Hi Segher, on 2021/11/30 上午8:16, Segher Boessenkool wrote: > Hi! > > On Tue, Sep 28, 2021 at 04:13:40PM +0800, Kewen.Lin wrote: >> PR target/102347 >> * config/rs6000/rs6000-call.c (rs6000_builtin_decl): Remove builtin >> mask check. > > (Don't wrap lines early please). > Fixed. > Okay for trunk and all backports. Thanks! > > Thanks for the review, re-based/re-tested and committed as r12-5590. Will backport it in a week or so. BR, Kewen
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; +}