From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id B4A803858D28 for ; Mon, 11 Oct 2021 06:35:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B4A803858D28 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 19B5ffmf031746; Mon, 11 Oct 2021 02:35:56 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3bmfbg8wwr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 11 Oct 2021 02:35:56 -0400 Received: from m0098420.ppops.net (m0098420.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 19B6RIXS011965; Mon, 11 Oct 2021 02:35:55 -0400 Received: from ppma06ams.nl.ibm.com (66.31.33a9.ip4.static.sl-reverse.com [169.51.49.102]) by mx0b-001b2d01.pphosted.com with ESMTP id 3bmfbg8ww6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 11 Oct 2021 02:35:55 -0400 Received: from pps.filterd (ppma06ams.nl.ibm.com [127.0.0.1]) by ppma06ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 19B6Rj3d010839; Mon, 11 Oct 2021 06:30:53 GMT Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by ppma06ams.nl.ibm.com with ESMTP id 3bk2bhtf29-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 11 Oct 2021 06:30:53 +0000 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 19B6UlW846399764 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 11 Oct 2021 06:30:47 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 642D44C062; Mon, 11 Oct 2021 06:30:47 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 4ABD24C073; Mon, 11 Oct 2021 06:30:44 +0000 (GMT) Received: from kewenlins-mbp.cn.ibm.com (unknown [9.200.146.131]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Mon, 11 Oct 2021 06:30:43 +0000 (GMT) Subject: Re: [PATCH] rs6000: Remove builtin mask check from builtin_decl [PR102347] To: Segher Boessenkool Cc: wschmidt@linux.ibm.com, David Edelsohn , bergner@linux.ibm.com, GCC Patches , will schmidt , Richard Biener 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> <20210930221350.GI22689@gate.crashing.org> From: "Kewen.Lin" Message-ID: <8defa60f-4192-1064-fcd3-d9a752d37aae@linux.ibm.com> Date: Mon, 11 Oct 2021 14:30:42 +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: <20210930221350.GI22689@gate.crashing.org> Content-Type: text/plain; charset=gbk Content-Language: en-US Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: 23Z9sepcIcyjv0suszOHsPOPzbIcaKuL X-Proofpoint-GUID: LzdRQQzt9F6UXAJTa1u-pOd0BwFttBsF 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-10-11_02,2021-10-07_02,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 lowpriorityscore=0 impostorscore=0 mlxscore=0 phishscore=0 mlxlogscore=999 clxscore=1015 malwarescore=0 suspectscore=0 bulkscore=0 adultscore=0 priorityscore=1501 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001 definitions=main-2110110038 X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, MIME_CHARSET_FARAWAY, 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: Mon, 11 Oct 2021 06:35:58 -0000 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 "\ \(" . ./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 "\ \(" . ./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