From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 99DEC3858C74 for ; Wed, 9 Mar 2022 19:24:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 99DEC3858C74 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-424-2xNxCa1-OleJyeteJpSO8g-1; Wed, 09 Mar 2022 14:24:32 -0500 X-MC-Unique: 2xNxCa1-OleJyeteJpSO8g-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3686D1006AA6; Wed, 9 Mar 2022 19:24:31 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.81]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E6FF160CA0; Wed, 9 Mar 2022 19:24:28 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 229JOPTf3194182 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 9 Mar 2022 20:24:25 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 229JOO1W3194181; Wed, 9 Mar 2022 20:24:24 +0100 Date: Wed, 9 Mar 2022 20:24:24 +0100 From: Jakub Jelinek To: Segher Boessenkool 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: Reply-To: Jakub Jelinek References: <20220307213718.GL614@gate.crashing.org> <20220309183406.GS614@gate.crashing.org> MIME-Version: 1.0 In-Reply-To: <20220309183406.GS614@gate.crashing.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham 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 19:24:36 -0000 On Wed, Mar 09, 2022 at 12:34:06PM -0600, Segher Boessenkool wrote: > It is a common idiom anyway, much clearer than any macro :-) (But no > parens please? sizeof is an operator, not a function). Ok, changed in my copy. > > > > + { "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 I agree, but I don't have much energy to fix all the bugs in the backend in one patch. Probably it could be fixed with incremental: --- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-03-09 19:55:31.879255798 +0100 +++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-09 20:10:53.778612345 +0100 @@ -713,9 +713,9 @@ rs6000_init_builtins (void) For IEEE 128-bit floating point, always create the type __ieee128. If the user used -mfloat128, rs6000-c.cc will create a define from __float128 to __ieee128. */ - if (TARGET_FLOAT128_TYPE) + if (TARGET_LONG_DOUBLE_128 && (!TARGET_IEEEQUAD || TARGET_FLOAT128_TYPE)) { - if (!TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128) + if (!TARGET_IEEEQUAD) ibm128_float_type_node = long_double_type_node; else { @@ -727,7 +727,12 @@ rs6000_init_builtins (void) t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST); lang_hooks.types.register_builtin_type (ibm128_float_type_node, "__ibm128"); + } + else + ibm128_float_type_node = NULL_TREE; + if (TARGET_FLOAT128_TYPE) + { if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128) ieee128_float_type_node = long_double_type_node; else @@ -737,7 +742,7 @@ rs6000_init_builtins (void) "__ieee128"); } else - ieee128_float_type_node = ibm128_float_type_node = NULL_TREE; + ieee128_float_type_node = NULL_TREE; /* Vector pair and vector quad support. */ vector_pair_type_node = make_node (OPAQUE_TYPE); @@ -3419,11 +3424,10 @@ rs6000_expand_builtin (tree exp, rtx tar return const0_rtx; } - if (bif_is_ibm128 (*bifaddr) - && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode))) + if (bif_is_ibm128 (*bifaddr) && !ibm128_float_type_node) { - error ("%qs requires %<__ibm128%> type support or % " - "to be IBM 128-bit format", bifaddr->bifname); + error ("%qs requires %<__ibm128%> type support", + bifaddr->bifname); return const0_rtx; } which ought to support __ibm128 as the same thing as long double if long double is double double, and also as a separate IFmode double double if -mabi=ieeelongdouble and TARGET_FLOAT128_TYPE (i.e. both IFmode and KFmode are supported). > > 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. True. Before the patch it means ibm128_float_type_node, which is a tree connected to __ibm128 type if it exists (but could very well be equal to long_double_type_node) or long_double_type if it doesn't. With the patch it means the same thing, except that ibm128_float_type_node is NULL if __ibm128 isn't supported, and at that point if means long_double_type_node. So, on the builtins side, if is the same thing as before. > > 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. TFmode isn't IFmode in that case, otherwise there wouldn't be the ICE on (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)) etc. IFmode and TFmode clearly are separate modes, which in those cases have the same REAL_MODE_FORMAT. So, under the hood they are handled the same, but still it isn't acceptable in RTL to mix the two modes in cases where RTL requires the same mode. Like in the SET_DEST/SET_SRC case must have the same mode, or operands of most arithmetic ops and the result have to have the same mode, ... > > 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. With the above incremental patch, perhaps (if it works). But still we can't make if be NULL if __ibm128 isn't supported, because we'll ICE already during creation of the builtins. Perhaps error_mark_node might be a possibility (though very risky), but because of the builtins generator that isn't an option (it appends "_type_node" and error_mark_node doesn't end with "_type_node"). Using long double will keep the existing behavior for that case and we'll reject calls to the builtin. > > 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 the above incremental patch I could use there __ibm128 instead of long double in the testcase (but even if I don't, there is just an extra long double to __ibm128 conversion which is either nop conversion or __float128 to __ibm128 conversion). What I wanted to say is that with e.g. -mlong-double-64 when there just isn't any double double there is nothing to pack or unpack, we should reject it rather than doing something weird. > > + 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. The incremental patch tweaks this. > > + 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. This is really necessary. I could check && TYPE_MODE (ibm128_float_type_node) == TFmode with the incremental patch if that is better understandable, but still, due to TFmode != IFmode it is required, and after all, it has been there before too, just done too late so that it already did a wrong thing before. Jakub