From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id A563B385841A for ; Thu, 30 Sep 2021 03:07:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A563B385841A Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 18U2UqSq016865; Wed, 29 Sep 2021 23:07:03 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3bd4gw8j17-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 29 Sep 2021 23:07:03 -0400 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 18U2gQZr020461; Wed, 29 Sep 2021 23:07:02 -0400 Received: from ppma06fra.de.ibm.com (48.49.7a9f.ip4.static.sl-reverse.com [159.122.73.72]) by mx0a-001b2d01.pphosted.com with ESMTP id 3bd4gw8j0d-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 29 Sep 2021 23:07:02 -0400 Received: from pps.filterd (ppma06fra.de.ibm.com [127.0.0.1]) by ppma06fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 18U2vFto027962; Thu, 30 Sep 2021 03:06:58 GMT Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by ppma06fra.de.ibm.com with ESMTP id 3b9u1kbx10-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 30 Sep 2021 03:06:58 +0000 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 18U36tnk62849448 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 30 Sep 2021 03:06:55 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2FD6411C05E; Thu, 30 Sep 2021 03:06:55 +0000 (GMT) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 24D4011C05C; Thu, 30 Sep 2021 03:06:52 +0000 (GMT) Received: from KewenLins-MacBook-Pro.local (unknown [9.200.53.210]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 30 Sep 2021 03:06:51 +0000 (GMT) Subject: Re: [PATCH] rs6000: Remove builtin mask check from builtin_decl [PR102347] To: wschmidt@linux.ibm.com Cc: Segher Boessenkool , David Edelsohn , bergner@linux.ibm.com, GCC Patches , will schmidt References: <7505a666-7b51-255c-9908-aabc753f7c33@linux.ibm.com> <62e6c096-f4a0-dc25-edba-ba0f32179438@linux.ibm.com> <10dc76e1-cf19-acc4-bb43-871ea87d3363@linux.ibm.com> From: "Kewen.Lin" Message-ID: Date: Thu, 30 Sep 2021 11:06:50 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <10dc76e1-cf19-acc4-bb43-871ea87d3363@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: B9WKRHHZq2QirAjkMTFelAbquuJ7WceN X-Proofpoint-ORIG-GUID: aqUVg6XV0nZqeQm095Sw5YoGRGxb5ocW X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475 definitions=2021-09-30_01,2021-09-29_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 phishscore=0 spamscore=0 adultscore=0 suspectscore=0 malwarescore=0 lowpriorityscore=0 priorityscore=1501 mlxscore=0 bulkscore=0 clxscore=1015 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001 definitions=main-2109300017 X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Sep 2021 03:07:09 -0000 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 >>>> >> >