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 AE31D3858297 for ; Tue, 6 Sep 2022 22:23:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AE31D3858297 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 286MMCXe012204; Tue, 6 Sep 2022 17:22:13 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 286MMCV6012203; Tue, 6 Sep 2022 17:22:12 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 6 Sep 2022 17:22:11 -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: <20220906222211.GH25951@gate.crashing.org> References: <20220823211345.GW25951@gate.crashing.org> 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 List-Id: On Fri, Sep 02, 2022 at 01:51:33PM -0400, Michael Meissner wrote: > On Tue, Aug 23, 2022 at 04:13:45PM -0500, Segher Boessenkool wrote: > > Please do not send new patches as replies to other patches. > > This was sent as a new patch. It probably was the partially copied to the mail body subject line that confused me. Sorry. > > 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. > > That is the first line from the git commit, which git format-patch puts as the > subject. I accidently deleted a few extra characters when trimming it down (I > remove the From:, etc. lines from the format-patch output). But I can just > delete this line if desired. Don't copy that line in the first place? Don't copy *anything* in fact, the way you copied stuff in that other patch (which is very hard to review, so it will have to wait a bit more) makes it impossible to apply the patch (which whould be just git-am). > > Please factor this patch into more pieces, pieces that can be reviewed > > more easily, pieces that change one thing only. Please do this. It is the biggest problem I have with most of your patches: you seem to save up development of a week, and then send it out as big omnibus patch an hour or two before my weekend. This is not ideal. > > > +(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. > > We know that TFmode (if -mabi=ieeelongdouble) and KFmode are the same, just > like TFmode (if -mabi=ibmlongdouble) and IFmode are the same. But RTL does not > know that these modes use the same representation. So to convert between them, > it needs to use either FLOAT_EXTEND or FLOAT_TRUNCATE, depending on which > precision each of the three modes have (i.e. rs6000-modes.h). So you need > these conversions in RTL. You have "extends" also when it is to a lower precision! Also, let me say this again: this needs to be solved. We cannot make real progress as long as we keep pretending every pait of floating point modes can be ordered. > Unfortunately, you can't just use SUBREG before register allocation is done. Why not? It isn't valid (in all cases) *after* RA, but before is fine. But you do not want a subreg, you need a conversion. > > But it certainly does not belong here in the middle of no-op moves. This is still very true. > So for example, for IBM floating point my current patches are: > But for the IEEE side, combining the two insns won't work, since going from > TFmode to KFmode will generate a FLOAT_TRUNCATE instead of a FLOAT_EXTEND. Yes. Please just fix the code depending on fundamentally wrong and unworkable assumptions, instead of adding perpetually worse workarounds that make things more rickety all the time. When you added this IF/KF/TF ordering stuff first, I was amazed that it worked as well as it did. And this was perhaps the best we could do for GCC 10, sure. But the problem should have been fixed since, there is no sane way forward without doing that. > > > +;; 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. > > That is how RTL goes between modes. Without a whole lot of changes to the > machine independent portion of the compiler, I don't see anyway of avoiding the > converts. It is not a conversion, not in RTL either. It changes mode, it doesn't change values. That is not the same thing. It's a subreg (one of the five overloaded meanings of subregs, but :-) ) > > > +(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! > > Various predicates do not allow SUBREG's of these types before register > allocation. Modes, not types, and I do not understand what you mean. Please show an example? Either way that is not a reason to do fully the wrong thing. It might make doing the right thing slightly more work, sure. Segher