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 200F03987502 for ; Thu, 10 Sep 2020 21:02:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 200F03987502 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@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 08AL1OHM021202; Thu, 10 Sep 2020 16:01:24 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 08AL1NZO021199; Thu, 10 Sep 2020 16:01:23 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 10 Sep 2020 16:01:23 -0500 From: Segher Boessenkool To: Michael Meissner , gcc-patches@gcc.gnu.org, David Edelsohn , Bill Schmidt , Peter Bergner Subject: Re: [PATCH 3/4] PowerPC: Add power10 xsmaxcqp/xsmincqp support Message-ID: <20200910210123.GS28786@gate.crashing.org> References: <20200827024142.GA15560@ibm-toto.the-meissners.org> <20200827024525.GC21803@ibm-toto.the-meissners.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200827024525.GC21803@ibm-toto.the-meissners.org> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 10 Sep 2020 21:02:26 -0000 Hi! On Wed, Aug 26, 2020 at 10:45:26PM -0400, Michael Meissner wrote: > * config/rs6000/rs6000.md (FSCALAR): New mode iterator for floating > point scalars. We have the long-established SFDF for a very similar thing. So maybe just call this SFDFTF or SDTF or something? Alternatively, we have FP for all the floating point modes -- maybe add a BFP that will then we just the (IEEE) binary floating point modes? > (Fm): New mode attribute for floating point scalars. Not happy about this at all. Just make *one* attribute that is the constraint to use for any mode? And if there are multiple choices for what constraint to use with some mode, well, hiding that behind a mode attribute is obfuscation then. The attribute always is "wa" nowadays (I should clean that up some day, heh). And is either "f" or "d", but those two constraints are identical these days. I have no idea what "F" means? (Or "m"!) > (s): Add support for the ISA 3.1 IEEE 128-bit > minimum and maximum instructions. > (s3_vsx): Add support for the ISA 3.1 IEEE 128-bit > minimum and maximum instructions. > +/* Macro whether the float128 min/max instructions are enabled. */ > +#define FLOAT128_IEEE_MINMAX_P(MODE) \ > + (TARGET_POWER10 && TARGET_FLOAT128_HW && FLOAT128_IEEE_P (MODE)) This is a horrible name. It is about the minmax*c* insns. And the "_P" is wrong (this macro is not checking if the mode is a minmax, whatever that would mean!) > +;; Constraints to use for scalar FP operations > +(define_mode_attr Fm [(SF "wa") > + (DF "wa") > + (TF "v") > + (KF "v")]) You miss a lot of context here... This of course is *not* for all scalar FP ops. Ah, so the "m" is for "minmax" I guess. > (define_insn "*s3_vsx" > - [(set (match_operand:SFDF 0 "vsx_register_operand" "=") > - (fp_minmax:SFDF (match_operand:SFDF 1 "vsx_register_operand" "") > - (match_operand:SFDF 2 "vsx_register_operand" "")))] > + [(set (match_operand:FSCALAR 0 "vsx_register_operand" "=") > + (fp_minmax:FSCALAR > + (match_operand:FSCALAR 1 "vsx_register_operand" "") > + (match_operand:FSCALAR 2 "vsx_register_operand" "")))] > "TARGET_VSX && TARGET_HARD_FLOAT" > { > + if (FLOAT128_IEEE_P (mode)) > + return "xscqp %0,%1,%2"; Nope. Since you need to handle this with completely separate code anyway, just make it a separate instruction pattern please. And there is no need for a define_expand for it, so you can just leave that one intact as-is. Segher