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 5CF253858D1E for ; Tue, 23 Aug 2022 21:14:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5CF253858D1E 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 27NLDkPI016262; Tue, 23 Aug 2022 16:13:46 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 27NLDjGd016261; Tue, 23 Aug 2022 16:13:45 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 23 Aug 2022 16:13:45 -0500 From: Segher Boessenkool To: Michael Meissner , gcc-patches@gcc.gnu.org, "Kewen.Lin" , David Edelsohn , Peter Bergner , Will Schmidt Subject: Re: [PATCH] Improve converting between 128-bit modes that use the same format Message-ID: <20220823211345.GW25951@gate.crashing.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.1 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.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 23 Aug 2022 21:14:50 -0000 Please do not send new patches as replies to other patches. On Thu, Aug 18, 2022 at 05:48:29PM -0400, Michael Meissner wrote: > mprove converting between 128-bit modes that use the same format. You are missing some characters? But this is an edited version of the subject anyway. Just don't do that (neither the copying or the editing), it just confuses things. Please factor this patch into more pieces, pieces that can be reviewed more easily, pieces that change one thing only. As is you are just rewriting the lot, and it is not an improvement at all this way. No doubt there are many good pieces in it, but mixed with a non-trivial amount of bad pieces I cannot approve it. It also isn't clear at all what you want to do; piece by piece it is easier to explain. > -; Iterators for converting to/from TFmode > -(define_mode_iterator IFKF [IF KF]) Yes, IFmode and KFmode have almost nothing in common. Good to see this go. It would be even better if we would not use rs6000_expand_float128_convert when not needed, either, and all this would be just gone after expand. > +(define_expand "extendkfif2" > + [(set (match_operand:IF 0 "gpc_reg_operand") > + (float_extend:IF (match_operand:KF 1 "gpc_reg_operand")))] > + "TARGET_FLOAT128_TYPE" > +{ > + rs6000_expand_float128_convert (operands[0], operands[1], false); > + DONE; > +}) This does not belong here. It really shouldn't *exist* at all: this is *not* a float_extend! It is not converting to a wider mode (as required!), but not even to a mode of higher precision: both IFmode and KFmode can represent (finite, normal) numbers the other can not. But it certainly does not belong here in the middle of no-op moves. > +(define_expand "trunckfif2" > + [(set (match_operand:IF 0 "gpc_reg_operand") > + (float_truncate:IF (match_operand:KF 1 "gpc_reg_operand")))] > + "TARGET_FLOAT128_TYPE" > +{ > + rs6000_expand_float128_convert (operands[0], operands[1], false); > + DONE; > +}) I also would expect IBM128 instead of just IF. This would simplify a lot. Why do you not use that, is there a reason? > +;; Convert between KFmode and TFmode when -mabi=ieeelongdouble > +(define_insn_and_split "*extendkftf2_internal" Same for IEEE128. And this isn't a conversion at all (it's a no-op move), please don't confuse things by saying it is. > + [(set_attr "type" "two") > + (set_attr "num_insns" "2")]) Btw, that really should never be needed. insn_type "two" already means exactly that. > +(define_insn_and_split "*extendtfif2_internal" > + [(set (match_operand:IF 0 "gpc_reg_operand" "=d,&d") > + (float_extend:IF > + (match_operand:TF 1 "input_operand" "0,d")))] > + "FLOAT128_IBM_P (TFmode)" > + "#" > + "&& reload_completed" Why would this ever need reload_completed? It is a no-op move! Segher