From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 02D50385F033 for ; Mon, 31 Jan 2022 21:33:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 02D50385F033 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 20VLWoY0022383; Mon, 31 Jan 2022 15:32:50 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 20VLWnGa022382; Mon, 31 Jan 2022 15:32:49 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 31 Jan 2022 15:32:49 -0600 From: Segher Boessenkool To: Bill Schmidt Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com Subject: Re: [PATCH 4/8] rs6000: Consolidate target built-ins code Message-ID: <20220131213249.GO614@gate.crashing.org> References: <9f4473f861d11ccc3bd11c05f37db041f849d8d6.1643390744.git.wschmidt@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9f4473f861d11ccc3bd11c05f37db041f849d8d6.1643390744.git.wschmidt@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no 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 21:33:53 -0000 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? > * 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. > +/* **** 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. Is it a big hassle to do that now, for this patch? Segher