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 DC9EF394443F for ; Tue, 13 Apr 2021 17:58:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DC9EF394443F 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 13DHvtEL029982; Tue, 13 Apr 2021 12:57:55 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 13DHvs2w029980; Tue, 13 Apr 2021 12:57:54 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 13 Apr 2021 12:57:54 -0500 From: Segher Boessenkool To: will schmidt Cc: Michael Meissner , gcc-patches@gcc.gnu.org, David Edelsohn , Bill Schmidt , Peter Bergner Subject: Re: [PATCH 1/2] Add IEEE 128-bit min/max support on PowerPC Message-ID: <20210413175754.GS26583@gate.crashing.org> References: <20210409143800.GA12782@ibm-toto.the-meissners.org> <20210409144250.GA14459@ibm-toto.the-meissners.org> <9dd6829db0b99cf325078d35f05f57263676e1da.camel@vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9dd6829db0b99cf325078d35f05f57263676e1da.camel@vnet.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-6.7 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: Tue, 13 Apr 2021 17:58:58 -0000 Hi! On Fri, Apr 09, 2021 at 11:54:57AM -0500, will schmidt wrote: > On Fri, 2021-04-09 at 10:42 -0400, Michael Meissner wrote: > > * config/rs6000/rs6000.c (rs6000_emit_minmax): Add support for ISA > > 3.1 IEEE 128-bit floating point xsmaxcqp and xsmincqp instructions. > > I don't see any direct reference to xsmaxcqp or xsmincqp with respect > to this change below. > > It looks like this change adds the FLOAT128_MIN_MAX_FPMASK_P (mode) > check > as criteria for emitting some form of a SET instruction. > emit_insn (gen_rtx_SET (dest, gen_rtx_fmt_ee (code, mode, op0, op1))); > > Ok, I see it now, the instructions are mildly obfuscated by > "xscqp" as part of the rs6000.md change below. Yeah, that is the downside of using iterators and the like in the machine description. But the upsides of that far outweigh these downsides :-) > > * config/rs6000/rs60000.h (FLOAT128_MIN_MAX_FPMASK_P): New macro. > > which is > #define FLOAT128_MIN_MAX_FPMASK_P(MODE) \ > (TARGET_POWER10 && TARGET_FLOAT128_HW && FLOAT128_IEEE_P (MODE)) > > Are there any non MIN_MAX scenarios that will require the combination > of POWER10,FLOAT128_HW,FLOAT128_IEEE(mode)? I'd wonder if there is a name > not specific to *_MIN_MAX_* that would be a better naming choice. > But, naming is hard. :-) Yes, and for every new macro, the reader will have to understand what it does, what it is for. If you cannot come up with a good name for it (one for which it is immediately obvious to the reader what it *means*), often a good tradeoff is to not make a macro at all, just write out the few words where you need them. > > * config/rs6000/rs6000.md (s3): Add support for the > > ISA 3.1 IEEE 128-bit minimum and maximum instructions. > > I'd move the "xsmaxcqp,xsmincqp" instruction references from the rs6000.c changelog blurb to this changelog blurb. It should say "New." or "New define_insns." or "New instructions." or similar. The changelog says *what*, not *why*. And it is important that you can find stuff in it using grep or similar. Here it should say "s3 for IEEE128". We actually have some patterns that just say "3", not too useful in a changelog if you do not say what code and mode are! (In this case, it does not help to say what "minmax" is from, it stand for just "min" and "max" after all :-) ) Segher