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 029F13858C74 for ; Wed, 9 Mar 2022 18:35:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 029F13858C74 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 229IY7lt030816; Wed, 9 Mar 2022 12:34:07 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 229IY6aO030815; Wed, 9 Mar 2022 12:34:06 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Wed, 9 Mar 2022 12:34:06 -0600 From: Segher Boessenkool To: Jakub Jelinek Cc: Michael Meissner , Jonathan Wakely , gcc-patches@gcc.gnu.org, David Edelsohn Subject: Re: [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708] Message-ID: <20220309183406.GS614@gate.crashing.org> References: <20220307213718.GL614@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.3 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: Wed, 09 Mar 2022 18:35:10 -0000 Hi! On Wed, Mar 09, 2022 at 02:27:19PM +0100, Jakub Jelinek wrote: > On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote: > > > 2) adjusts the builtins code to use > > > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node > > > for the 2 builtins, so that we don't ICE during builtins creation > > > if ibm128_float_type_node is NULL (using error_mark_node instead of > > > long_double_type_node sounds more risky to me) > > > > I feel the opposite way: (potentially) using the wrong thing is just a > > ticking time bomb, never "safer". > > Actually, fallback to long_double_type_node is the right thing, see below. I don't see how that ever could be true, but I'll see below perhaps :-) > > There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-) > > Ok, added now. Thanks! > > The basic types, that always exist if they are supported at all, are > > __ibm128 and __ieee128. Long double picks one of those, or some other > > type; and __float128 is a source of confusion (it tries to make things > > more similar to x86, but things are *not* similar anyway :-( ) > > Not just x86, there are multiple targets with __float128 support, and > when it works, it behaves the same on all of them. Unfortunately it isn't documented anywhere *what* it should mean, then. > Mike's PR99708 patch (which unfortunately has been backported to various > branches without resolving these issues first) regressed on powerpc64-linux It supposedly was tested there though? Sigh. > > Tested with which -mcpu=? You need at least power7 to get __ieee128 > > support, but it should be tested *without* that as well. (The actual > > default for powerpc64-linux is power4 (so not even 970), and for > > powerpc-linux it is 603 or such.) > > ../configure --enable-languages=c,c++,fortran,objc,obj-c++,lto,go --prefix=/home/jakub/GCC; make -j64 > LOG 2>&1 && make -j64 -k check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}' > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1 > i.e. the oldest supported one. On powerpc64le-linux also without any > --with- tuning, so power8. So power4 for powerpc64-linux, and power8 for powerpc64le-linux. I ask because many people do use --with-cpu=, and the difference matters a lot here. > > > * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to > > > 85 instead of 86. > > > > This should be auto-generated, or just not exist at all > > ("sizeof type_map / sizeof type_map[0]" in the one place it is used, > > instead -- "one place" is after removing it from the definition of the > > array of course, where it is unneeded :-) ) > > Unfortunately the generator doesn't use libiberty.h, so I couldn't use > ARRAY_SIZE. Just used sizeof / sizeof [0]. It is a common idiom anyway, much clearer than any macro :-) (But no parens please? sizeof is an operator, not a function). > > > + { "if", "ibm128_float_type_node " > > > + "? ibm128_float_type_node " > > > + ": long_double" }, > > > > I don't think that is correct. If there is no __ibm128, there is no > > IFmode either (they are two sides of the same coin). Using DFmode > > instead (which is the only thing that "long double" could be if not > > IFmode) will just ICE later, if this is used at all. So best case this > > will confuse the reader. > > rs6000_expand_builtin has (but at an incorrect spot) code to remap > __builtin_{,un}pack_ibm128 to __builtin_{,un}pack_longdouble under the hood, > for 2 reasons: > 1) __ibm128 type is only supported when VSX is enabled, Ugh, another bug. *^*ET*(^(*^TE($^TW(*T(W > seems the intent > was that those 2 builtins can be used interchangeably unless > long double is the same as __ieee128, in which case > __builtin_{,un}pack_longdouble errors and > __builtin_{,un}pack_ibm128 works and returns/takes __ibm128 Aha. But the type "if" always means the same thing, in the builtins language. > 2) even with VSX, unless -mlong-double-128 -mabi=ieeelongdouble is in > effect, ibm128_float_type_node has TFmode rather than IFmode, > so we can't use the {,un}packif expanders but need {,un}packtf > instead TFmode *is* IFmode, in that case. TFmode is a workaround for shortcomings in the generic parts of GCC. This almost works as well even! But it is confusing no end. > Which means the > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node > is actually the right thing (except for -mlong-double-64 but the > patch will reject those builtins in that case) - if > ibm128_float_type_node is NULL but long_double_type_node is TFmode, > we want __builtin_{,un}pack_ibm128 to work like > __builtin_{,un}pack_longdouble and use long double, if > ibm128_float_type_node is non-NULL and long_double_type_node is TFmode, > ibm128_float_type_node == long_double_type_node and again we want it to > work like __builtin_{,un}pack_longdouble and finally if > ibm128_float_type_node is non-NULL and long_double_type_node is KFmode, > it will use ibm128_float_type_node with IFmode. ibm128_float_type_node should always be defined if we can use double- double. That is the whole point of having __ibm128 and __ieee128 types at all: they exist whenever they can, and always mean the same thing. We should never (have to) do strange gymnastics to access those types. > E.g. C++ > auto foo (void) { return __builtin_pack_ibm128 (100000000.0, 0.0000000001); } > double bar (long double x) { return __builtin_unpack_ibm128 (x, 0); } > double baz (long double x) { return __builtin_unpack_ibm128 (x, 1); } > used to be strangely accepted with -mlong-double-64 but did just a weird > thing (now is rejected), Neither is correct; it should do the simple and correct thing in both cases. Whatever your long double is: long double has nothing whatsoever to do with __ibm128 and __ieee128. > with -mlong-double-128 without VSX it used to ICE, > e.g. with > error: unrecognizable insn: > 5 | } > | ^ > (insn 9 8 10 2 (set (reg:IF 119) > (unspec:TF [ > (reg:DF 120) > (reg:DF 122) > ] UNSPEC_PACK_128BIT)) "prqquu.C":4:32 -1 > (nil)) > and now works, ditto used to ICE with -mlong-double-128 -mcpu=power8 etc., > and just worked and keeps working with -mlong-double-128 -mabi=ieeelongdouble. > + if (bif_is_ibm128 (*bifaddr) > + && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode))) > + { > + error ("%qs requires %<__ibm128%> type support or % " > + "to be IBM 128-bit format", bifaddr->bifname); > + return const0_rtx; > + } So this part is wrong imo. > + if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD) > + { > + if (fcode == RS6000_BIF_PACK_IF) > + { > + icode = CODE_FOR_packtf; > + fcode = RS6000_BIF_PACK_TF; > + uns_fcode = (size_t) fcode; > + } > + else if (fcode == RS6000_BIF_UNPACK_IF) > + { > + icode = CODE_FOR_unpacktf; > + fcode = RS6000_BIF_UNPACK_TF; > + uns_fcode = (size_t) fcode; > + } > + } And this. Thank you for dealing with this jumble! Segher