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 4C721385840C for ; Fri, 28 Jan 2022 23:10:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4C721385840C 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 20SN9UHw013894; Fri, 28 Jan 2022 17:09:30 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 20SN9Tom013888; Fri, 28 Jan 2022 17:09:29 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 28 Jan 2022 17:09:29 -0600 From: Segher Boessenkool To: Bill Schmidt Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com Subject: Re: [PATCH 1/8] rs6000: More factoring of overload processing Message-ID: <20220128230929.GI614@gate.crashing.org> References: <9ee506e947ec49973f757ea4a967574ded4ed2b0.1643390744.git.wschmidt@linux.ibm.com> <20220128191110.GG614@gate.crashing.org> <4b46e4f0-f186-3796-a4df-14fd57fea33a@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4b46e4f0-f186-3796-a4df-14fd57fea33a@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: Fri, 28 Jan 2022 23:10:34 -0000 On Fri, Jan 28, 2022 at 03:19:48PM -0600, Bill Schmidt wrote: > On 1/28/22 1:11 PM, Segher Boessenkool wrote: > > On Fri, Jan 28, 2022 at 11:50:19AM -0600, Bill Schmidt wrote: > >> + and the generic code will issue the appropriate error message. Skip > >> + this test for functions where we don't fully describe all the possible > >> + overload signatures in rs6000-overload.def (because they aren't relevant > >> + to the expansion here). If we don't, we get confusing error messages. */ > >> + if (fcode != RS6000_OVLD_VEC_PROMOTE > >> + && fcode != RS6000_OVLD_VEC_SPLATS > >> + && fcode != RS6000_OVLD_VEC_EXTRACT > >> + && fcode != RS6000_OVLD_VEC_INSERT > >> + && fcode != RS6000_OVLD_VEC_STEP > >> + && (!VOID_TYPE_P (TREE_VALUE (fnargs)) || n < nargs)) > >> return NULL; > > Can you expand a bit on this, give an example for example? It is very > > hard to understand this code, the way it depends on code following many > > lines later. > > Sure, sorry. > > This check gives up if the number of arguments doesn't match the prototype. > It gives a fairly generic error message. That part of it has always been > in here. > > Now, I moved this check forward relative to the big switch statement on > fcode, because there are redundant checks for the number of arguments > in each of the resolve_vec_* helper functions. This allowed me to simplify > those a bit. > > Now, it turns out that this doesn't work so well for functions that aren't > fully described in rs6000-overload.def. For example, for vec_splats we > have: > > ; There are no actual builtins for vec_splats. There is special handling for > ; this in altivec_resolve_overloaded_builtin in rs6000-c.cc, where the call > ; is replaced by a constructor. The single overload here causes > ; __builtin_vec_splats to be registered with the front end so that can happen. > [VEC_SPLATS, vec_splats, __builtin_vec_splats] > vsi __builtin_vec_splats (vsi); > ABS_V4SI SPLATS_FAKERY > > So even though __builtin_vec_splats accepts all vector types, the > infrastructure cheats and just records one prototype. We end up getting > an error message that refers to this specific prototype even when we are > handling a different argument type. That is completely confusing to the > user. So I felt I was starting to get too deep for a simple refactoring > patch, and gave up on early number-of-arguments checking for the special > cases that use the _FAKERY technique. > > That's probably still not clear, but maybe clearer? Much better, thanks! So put a comment before the code handling the arg checking for vec_splats etc. saying just that? Or the much condensed form "these codes should be handled separately because " :-) (And the larger explanation in the commit message -- there you can talk about the old code / old situation as well :-) ) > >> + default: > >> + ; > > Don't. > > > > I like this better than a BS break statement, but it is just as stupid. > > > > If you need this, you don't want a switch statement, but some number of > > if statements. You cannot use a switch as a shorthand for this because > > we have a silly warning and -Werror for this use. > > > > You probably get easier to understand code that way, too, you can get > > rid of the above (just do some early returns), etc. > > If I understand correctly, you'd like me to resubmit this in if-then-else > form. That's fine, just want to be sure that's what you want. Yes please. This is new code, so let's please keep it as readable as possible. Since you need to redo some of it anyway as well... Segher