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 A22EA3858D35 for ; Mon, 31 Jan 2022 22:06:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A22EA3858D35 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 20VM6e1j000930; Mon, 31 Jan 2022 22:06:43 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3dxkth69vy-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 Jan 2022 22:06:42 +0000 Received: from m0098416.ppops.net (m0098416.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 20VM6fe4001083; Mon, 31 Jan 2022 22:06:41 GMT Received: from ppma03wdc.us.ibm.com (ba.79.3fa9.ip4.static.sl-reverse.com [169.63.121.186]) by mx0b-001b2d01.pphosted.com with ESMTP id 3dxkth69km-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 Jan 2022 22:06:41 +0000 Received: from pps.filterd (ppma03wdc.us.ibm.com [127.0.0.1]) by ppma03wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 20VLqS1X017778; Mon, 31 Jan 2022 22:02:00 GMT Received: from b01cxnp22034.gho.pok.ibm.com (b01cxnp22034.gho.pok.ibm.com [9.57.198.24]) by ppma03wdc.us.ibm.com with ESMTP id 3dvw7a4p07-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 Jan 2022 22:02:00 +0000 Received: from b01ledav003.gho.pok.ibm.com (b01ledav003.gho.pok.ibm.com [9.57.199.108]) by b01cxnp22034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 20VM1xC438011144 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 31 Jan 2022 22:01:59 GMT Received: from b01ledav003.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9C0F7B206A; Mon, 31 Jan 2022 22:01:59 +0000 (GMT) Received: from b01ledav003.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 37E24B2067; Mon, 31 Jan 2022 22:01:59 +0000 (GMT) Received: from [9.211.95.53] (unknown [9.211.95.53]) by b01ledav003.gho.pok.ibm.com (Postfix) with ESMTP; Mon, 31 Jan 2022 22:01:59 +0000 (GMT) Message-ID: Date: Mon, 31 Jan 2022 16:01:58 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.5.1 Reply-To: wschmidt@linux.ibm.com Subject: Re: [PATCH 4/8] rs6000: Consolidate target built-ins code To: Segher Boessenkool Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com References: <9f4473f861d11ccc3bd11c05f37db041f849d8d6.1643390744.git.wschmidt@linux.ibm.com> <20220131213249.GO614@gate.crashing.org> From: Bill Schmidt In-Reply-To: <20220131213249.GO614@gate.crashing.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: 7W40cQmgcVHrcobOeCo4pLMa6mRFx4Ba X-Proofpoint-GUID: 0BM6Rs5wxKKjmiPLpt0evpt5tJW1-THd X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2022-01-31_07,2022-01-31_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 suspectscore=0 mlxlogscore=999 clxscore=1015 phishscore=0 lowpriorityscore=0 spamscore=0 adultscore=0 bulkscore=0 mlxscore=0 impostorscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000 definitions=main-2201310139 X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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, 31 Jan 2022 22:06:45 -0000 Hi Segher, On 1/31/22 3:32 PM, Segher Boessenkool wrote: > Hi! > > On Fri, Jan 28, 2022 at 11:50:22AM -0600, Bill Schmidt wrote: >> Continuing with the refactoring effort, this patch moves as much of the >> target-specific built-in support code into a new file, rs6000-builtin.cc. >> However, we can't easily move the overloading support code out of >> rs6000-c.cc, because the build machinery understands that as a special file >> to be included with the C and C++ front ends. > And the other C-like frontends. > >> This patch is just a straightforward move, with one exception. I found >> that the builtin_mode_to_type[] array is no longer used, so I also removed >> all code having to do with it. > Oh nice, your rewrite removed the need for that array. Great :-) > >> The code in rs6000-builtin.cc is organized in related sections: >> - General support functions >> - Initialization support >> - GIMPLE folding support >> - Expansion support >> >> Overloading support remains in rs6000-c.cc. > So, what is needed to move that as well? Is moving that in the plan? No, as explained above, that code needs to stay in the "special" file that the build machinery understands. It looks very difficult to tease that apart, so I've given up on that. Sorry! > >> * config/rs6000/rs6000-builtin.cc: New file, containing code moved >> from other files. > (You're breaking lines early again.) > >> - extra_objs="${extra_objs} rs6000-builtins.o" >> + extra_objs="${extra_objs} rs6000-builtins.o rs6000-builtin.o" > It's pretty unfortunate that these files are named alike. The source > files exist in different places of course, so the danger of confusion > is minimal usually. > >> +/* Support targetm.vectorize.builtin_mask_for_load. */ >> +tree altivec_builtin_mask_for_load; > "Support"? What does that mean? Please describe what this tree is. That comment is just moved. There's a target hook from the vectorizer for Altivec-style unaligned load masking. The target needs to provide a built-in function for this. The tree contains its function decl. I can change the comment. > >> +/* **** General support functions. **** */ > This isn't a sentence so should not have a full stop. (And otherwise > it should be followed by two spaces!) > >> +bool >> +rs6000_builtin_is_supported (enum rs6000_gen_builtins fncode) >> +{ >> + switch (rs6000_builtin_info[(size_t) fncode].enable) >> + { >> + case ENB_ALWAYS: >> + return true; >> + case ENB_P5: >> + return TARGET_POPCNTB; >> + case ENB_P6: >> + return TARGET_CMPB; >> + case ENB_P6_64: >> + return TARGET_CMPB && TARGET_POWERPC64; >> + case ENB_P7: >> + return TARGET_POPCNTD; >> + case ENB_P7_64: >> + return TARGET_POPCNTD && TARGET_POWERPC64; >> + case ENB_P8: >> + return TARGET_DIRECT_MOVE; >> + case ENB_P8V: >> + return TARGET_P8_VECTOR; >> + case ENB_P9: >> + return TARGET_MODULO; >> + case ENB_P9_64: >> + return TARGET_MODULO && TARGET_POWERPC64; >> + case ENB_P9V: >> + return TARGET_P9_VECTOR; >> + case ENB_P10: >> + return TARGET_POWER10; >> + case ENB_P10_64: >> + return TARGET_POWER10 && TARGET_POWERPC64; >> + case ENB_ALTIVEC: >> + return TARGET_ALTIVEC; >> + case ENB_VSX: >> + return TARGET_VSX; >> + case ENB_CELL: >> + return TARGET_ALTIVEC && rs6000_cpu == PROCESSOR_CELL; >> + case ENB_IEEE128_HW: >> + return TARGET_FLOAT128_HW; >> + case ENB_DFP: >> + return TARGET_DFP; >> + case ENB_CRYPTO: >> + return TARGET_CRYPTO; >> + case ENB_HTM: >> + return TARGET_HTM; >> + case ENB_MMA: >> + return TARGET_MMA; >> + default: >> + gcc_unreachable (); >> + } >> + gcc_unreachable (); >> +} > If you rewrite this without switch it is shorter and clearer, and you do > not need to duplicate the gcc_unreachable (which the broken warning > forces you to). > >> + if (fcode >= RS6000_OVLD_MAX) >> + return error_mark_node; > This shows that that isn't really the max, it is the number of elts in > the array, instead (maximum is inclusive). Maybe fis that some day :-) > >> +/* Implement targetm.vectorize.builtin_md_vectorized_function. */ >> + >> +tree >> +rs6000_builtin_md_vectorized_function (tree fndecl, tree type_out, >> + tree type_in) >> +{ >> + machine_mode in_mode, out_mode; >> + int in_n, out_n; >> + >> + if (TARGET_DEBUG_BUILTIN) >> + fprintf (stderr, >> + "rs6000_builtin_md_vectorized_function (%s, %s, %s)\n", >> + IDENTIFIER_POINTER (DECL_NAME (fndecl)), >> + GET_MODE_NAME (TYPE_MODE (type_out)), >> + GET_MODE_NAME (TYPE_MODE (type_in))); >> + >> + /* TODO: Should this be gcc_assert? */ >> + if (TREE_CODE (type_out) != VECTOR_TYPE >> + || TREE_CODE (type_in) != VECTOR_TYPE) >> + return NULL_TREE; > Yes, as target.def says. > >> + enum rs6000_gen_builtins fn >> + = (enum rs6000_gen_builtins) DECL_MD_FUNCTION_CODE (fndecl); > You could write > = (decltype (fn)) DECL_MD_FUNCTION_CODE (fndecl); > btw. > >> + default: >> + break; > Another wart :-( > > The -Wswitch* warnings this works around should not be part of -Wall > imo, not even of -Wextra :-( > >> +static >> +const char *rs6000_type_string (tree type_node) >> +{ >> + if (type_node == void_type_node) >> + return "void"; >> + else if (type_node == long_integer_type_node) >> + return "long"; > You do not need the "else" btw. Getting rid of that will make the code > easier to read. > >> +void >> +rs6000_init_builtins (void) >> +{ >> + tree tdecl; >> + tree t; > Don't declare things long before they are used please. > > ... > > Bah, I am commenting on existing code... Please do not mix code > movement and other things in the same patch. In the (very few!) cases > where you need things to stay together for bisectability, you can still > submit it as multiple patches (and you have that, that is how you write > it in the first place!), just comment that you need to check in a few > patches together. This entire patch is just moving code. I already separated that out from everything else. The only exception is the one I called out at the beginning, that removing the builtin_mode_to_type[] array should have been done previously, and it's convenient to just delete it as part of this patch. So I can change these other things that you mention, but that isn't really the point of this patch, so... I guess please advise. :-) Thanks, Bill > > Is it a big hassle to do that now, for this patch? > > > Segher