Message ID | 718677e7-614d-7977-312d-05a75e1fd5b4@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id ECA62385843D for <patchwork@sourceware.org>; Wed, 21 Dec 2022 09:25:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org ECA62385843D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1671614740; bh=X0fyTaMIvJk8hs0kcz7/CsAae7bhrXoZUW0nqWatJtE=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=j5rSbTO8arq6VcoV0+h4kVzMj4ns3aXMtaLG793nqMrq9VN52d+P/mGzH9BnIeaEF GDmq0IsL6e/Eazksb+9bwAcnXJeXZJMh/+YE0rHqOJqL9BSWFTNQWaXn49DfEcVwcr ffnDXraVAd1E92EyZk83GrQUv2YHECE60+f6+Ux8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 7FDED3858D1E for <gcc-patches@gcc.gnu.org>; Wed, 21 Dec 2022 09:25:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7FDED3858D1E Received: from pps.filterd (m0098396.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2BL9FBd2021020; Wed, 21 Dec 2022 09:24:55 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3mkybkg7j2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Dec 2022 09:24:55 +0000 Received: from m0098396.ppops.net (m0098396.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 2BL9FMx3021464; Wed, 21 Dec 2022 09:24:54 GMT Received: from ppma04fra.de.ibm.com (6a.4a.5195.ip4.static.sl-reverse.com [149.81.74.106]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3mkybkg7dn-9 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Dec 2022 09:24:54 +0000 Received: from pps.filterd (ppma04fra.de.ibm.com [127.0.0.1]) by ppma04fra.de.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 2BKNC9Is027900; Wed, 21 Dec 2022 09:02:26 GMT Received: from smtprelay01.fra02v.mail.ibm.com ([9.218.2.227]) by ppma04fra.de.ibm.com (PPS) with ESMTPS id 3mh6yw3tff-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 21 Dec 2022 09:02:26 +0000 Received: from smtpav04.fra02v.mail.ibm.com (smtpav04.fra02v.mail.ibm.com [10.20.54.103]) by smtprelay01.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 2BL92MLA49348880 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 21 Dec 2022 09:02:22 GMT Received: from smtpav04.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3B9FE20049; Wed, 21 Dec 2022 09:02:22 +0000 (GMT) Received: from smtpav04.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 1C7CB20043; Wed, 21 Dec 2022 09:02:19 +0000 (GMT) Received: from [9.197.239.17] (unknown [9.197.239.17]) by smtpav04.fra02v.mail.ibm.com (Postfix) with ESMTP; Wed, 21 Dec 2022 09:02:18 +0000 (GMT) Message-ID: <718677e7-614d-7977-312d-05a75e1fd5b4@linux.ibm.com> Date: Wed, 21 Dec 2022 17:02:17 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Content-Language: en-US To: GCC Patches <gcc-patches@gcc.gnu.org> Cc: Michael Meissner <meissner@linux.ibm.com>, Segher Boessenkool <segher@kernel.crashing.org>, David Edelsohn <dje.gcc@gmail.com>, Jakub Jelinek <jakub@redhat.com>, Joseph Myers <joseph@codesourcery.com>, Peter Bergner <bergner@linux.ibm.com>, Richard Biener <richard.guenther@gmail.com>, Richard Sandiford <richard.sandiford@arm.com> Subject: [RFC/PATCH] Remove the workaround for _Float128 precision [PR107299] Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: o0H8O6x8vg7ggTmBqHbBMdbFlv3uw0Zg X-Proofpoint-GUID: IpptQucEYREDdOXEZYWBbowVoLwTp_r1 Content-Transfer-Encoding: 7bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-12-21_04,2022-12-20_01,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 malwarescore=0 impostorscore=0 suspectscore=0 lowpriorityscore=0 phishscore=0 spamscore=0 mlxlogscore=999 adultscore=0 priorityscore=1501 bulkscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2212210072 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: "Kewen.Lin via Gcc-patches" <gcc-patches@gcc.gnu.org> Reply-To: "Kewen.Lin" <linkw@linux.ibm.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[RFC/PATCH] Remove the workaround for _Float128 precision [PR107299]
|
|
Commit Message
Kewen.Lin
Dec. 21, 2022, 9:02 a.m. UTC
Hi, This a different attempt from Mike's approach[1][2] to fix the issue in PR107299. With option -mabi=ieeelongdouble specified, type long double (and __float128) and _Float128 have the same mode TFmode, but they have different type precisions, it causes the assertion to fail in function fold_using_range::fold_stmt. IMHO, it's not sensible that two types have the same mode but different precisions. By tracing where we make type _Float128 have different precision from the precision of its mode, I found it's from a work around for rs6000 KFmode. Being curious why we need this work around, I disabled it and tested it with some combinations as below, but all were bootstrapped and no regression failures were observed. - BE Power8 with --with-long-double-format=ibm - LE Power9 with --with-long-double-format=ibm - LE Power10 with --with-long-double-format=ibm - x86_64-redhat-linux - aarch64-linux-gnu For LE Power10 with --with-long-double-format=ieee, since it suffered the bootstrapping issue, I compared the regression testing results with the one from Mike's approach. The comparison result showed this approach can have several more test cases pass and no cases regressed, they are: 1) libstdc++: FAIL->PASS: std/format/arguments/args.cc (test for excess errors) FAIL->PASS: std/format/error.cc (test for excess errors) FAIL->PASS: std/format/formatter/requirements.cc (test for excess errors) FAIL->PASS: std/format/functions/format.cc (test for excess errors) FAIL->PASS: std/format/functions/format_to_n.cc (test for excess errors) FAIL->PASS: std/format/functions/size.cc (test for excess errors) FAIL->PASS: std/format/functions/vformat_to.cc (test for excess errors) FAIL->PASS: std/format/parse_ctx.cc (test for excess errors) FAIL->PASS: std/format/string.cc (test for excess errors) FAIL->PASS: std/format/string_neg.cc (test for excess errors) Caused by the same reason: one static assertion fail in header file format (_Type is __ieee128): static_assert(__format::__formattable_with<_Type, _Context>); 2) gfortran: NA->PASS: gfortran.dg/c-interop/typecodes-array-float128.f90 NA->PASS: gfortran.dg/c-interop/typecodes-scalar-float128.f90 NA->PASS: gfortran.dg/PR100914.f90 Due to that the effective target `fortran_real_c_float128` checking fails, fail to compile below source with error msg: "Error: Kind -4 not supported for type REAL". use iso_c_binding real(kind=c_float128) :: x x = cos (x) end 3) g++: FAIL->PASS: g++.dg/cpp23/ext-floating1.C -std=gnu++23 Due to the static assertion failing: static_assert (is_same<decltype (0.0q), __float128>::value); * compatible with long double This approach keeps type long double compatible with _Float128 (at -mabi=ieeelongdouble) as before, so for the simple case like: _Float128 foo (long double t) { return t; } there is no conversion. See the difference at optimized dumping: with the contrastive approach: _Float128 foo (long double a) { _Float128 _2; <bb 2> [local count: 1073741824]: _2 = (_Float128) a_1(D); return _2; } with this: _Float128 foo (long double a) { <bb 2> [local count: 1073741824]: return a_1(D); } IMHO, it's still good to keep _Float128 and __float128 compatible with long double, to get rid of those useless type conversions. Besides, this approach still takes TFmode attribute type as type long double, while the contrastive approach takes TFmode attribute type as type _Float128, whose corresponding mode isn't TFmode, the inconsistence seems not good. As above, I wonder if we can consider this approach which makes type _Float128 have the same precision as MODE_PRECISION of its mode, it keeps the previous implementation to make type long double compatible with _Float128. Since the REAL_MODE_FORMAT of the mode still holds the information, even if some place which isn't covered in the above testing need the actual precision, we can still retrieve the actual precision with that. Any thoughts? [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604834.html [2] v3 https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608513.html BR, Kewen ----- PR target/107299 gcc/ChangeLog: * tree.cc (build_common_tree_nodes): Remove workaround for rs6000 KFmode. --- gcc/tree.cc | 9 --------- 1 file changed, 9 deletions(-) -- 2.27.0
Comments
Hi! On Wed, Dec 21, 2022 at 05:02:17PM +0800, Kewen.Lin wrote: > This a different attempt from Mike's approach[1][2] to fix the > issue in PR107299. Ke Wen, Mike: so iiuc with this patch applied all three of Mike's patches are unnecessary? > With option -mabi=ieeelongdouble specified, > type long double (and __float128) and _Float128 have the same > mode TFmode, but they have different type precisions, it causes > the assertion to fail in function fold_using_range::fold_stmt. > IMHO, it's not sensible that two types have the same mode but > different precisions. Yes, absolutely. It is a hack, and always was a hack, just one that worked remarkably well. But the problems it worked around have not been fixed yet (and the workarounds are not perfect either). > By tracing where we make type _Float128 > have different precision from the precision of its mode, I found > it's from a work around for rs6000 KFmode. Being curious why > we need this work around, I disabled it and tested it with some > combinations as below, but all were bootstrapped and no > regression failures were observed. > > - BE Power8 with --with-long-double-format=ibm > - LE Power9 with --with-long-double-format=ibm > - LE Power10 with --with-long-double-format=ibm > - x86_64-redhat-linux > - aarch64-linux-gnu > > For LE Power10 with --with-long-double-format=ieee, since it > suffered the bootstrapping issue, I compared the regression > testing results with the one from Mike's approach. The > comparison result showed this approach can have several more > test cases pass and no cases regressed, they are: > > 1) libstdc++: > > FAIL->PASS: std/format/arguments/args.cc (test for excess errors) > FAIL->PASS: std/format/error.cc (test for excess errors) > FAIL->PASS: std/format/formatter/requirements.cc (test for excess errors) > FAIL->PASS: std/format/functions/format.cc (test for excess errors) > FAIL->PASS: std/format/functions/format_to_n.cc (test for excess errors) > FAIL->PASS: std/format/functions/size.cc (test for excess errors) > FAIL->PASS: std/format/functions/vformat_to.cc (test for excess errors) > FAIL->PASS: std/format/parse_ctx.cc (test for excess errors) > FAIL->PASS: std/format/string.cc (test for excess errors) > FAIL->PASS: std/format/string_neg.cc (test for excess errors) > > Caused by the same reason: one static assertion fail in header > file format (_Type is __ieee128): > > static_assert(__format::__formattable_with<_Type, _Context>); > > 2) gfortran: > > NA->PASS: gfortran.dg/c-interop/typecodes-array-float128.f90 > NA->PASS: gfortran.dg/c-interop/typecodes-scalar-float128.f90 > NA->PASS: gfortran.dg/PR100914.f90 > > Due to that the effective target `fortran_real_c_float128` > checking fails, fail to compile below source with error msg: > "Error: Kind -4 not supported for type REAL". > > use iso_c_binding > real(kind=c_float128) :: x > x = cos (x) > end > > 3) g++: > > FAIL->PASS: g++.dg/cpp23/ext-floating1.C -std=gnu++23 > > Due to the static assertion failing: > > static_assert (is_same<decltype (0.0q), __float128>::value); Does it fix the new testcases in Mike's series as well? > * compatible with long double > > This approach keeps type long double compatible with _Float128 > (at -mabi=ieeelongdouble) as before, so for the simple case > like: > > _Float128 foo (long double t) { return t; } > > there is no conversion. See the difference at optimized > dumping: > > with the contrastive approach: > > _Float128 foo (long double a) > { > _Float128 _2; > > <bb 2> [local count: 1073741824]: > _2 = (_Float128) a_1(D); > return _2; > } > > with this: > > _Float128 foo (long double a) > { > <bb 2> [local count: 1073741824]: > return a_1(D); > } > > IMHO, it's still good to keep _Float128 and __float128 > compatible with long double, to get rid of those useless > type conversions. > > Besides, this approach still takes TFmode attribute type > as type long double, while the contrastive approach takes > TFmode attribute type as type _Float128, whose corresponding > mode isn't TFmode, the inconsistence seems not good. > > As above, I wonder if we can consider this approach which > makes type _Float128 have the same precision as MODE_PRECISION > of its mode, it keeps the previous implementation to make type > long double compatible with _Float128. Since the REAL_MODE_FORMAT > of the mode still holds the information, even if some place which > isn't covered in the above testing need the actual precision, we > can still retrieve the actual precision with that. "Precision" does not have a useful meaning for all floating point formats. It does not have one for double-double for example. The way precision is defined in IEEE FP means double-double has 2048 bits of precision (or is it 2047), not useful. Taking the precision of the format instead to be the minimum over all values in that format gives double-double the same precision as IEEE DP, not useful in any practical way either :-/ > --- a/gcc/tree.cc > +++ b/gcc/tree.cc > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > if (!targetm.floatn_mode (n, extended).exists (&mode)) > continue; > int precision = GET_MODE_PRECISION (mode); > - /* Work around the rs6000 KFmode having precision 113 not > - 128. */ It has precision 126 now fwiw. Joseph: what do you think about this patch? Is the workaround it removes still useful in any way, do we need to do that some other way if we remove this? Segher
On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > --- a/gcc/tree.cc > > +++ b/gcc/tree.cc > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > continue; > > int precision = GET_MODE_PRECISION (mode); > > - /* Work around the rs6000 KFmode having precision 113 not > > - 128. */ > > It has precision 126 now fwiw. > > Joseph: what do you think about this patch? Is the workaround it > removes still useful in any way, do we need to do that some other way if > we remove this? I think it's best for the TYPE_PRECISION, for any type with the binary128 format, to be 128 (not 126). It's necessary that _Float128, _Float64x and long double all have the same TYPE_PRECISION when they have the same (binary128) format, or at least that TYPE_PRECISION for _Float128 >= that for long double >= that for _Float64x, so that the rules in c_common_type apply properly. How the TYPE_PRECISION compares to that of __ibm128, or of long double when that's double-double, is less important.
On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > > --- a/gcc/tree.cc > > > +++ b/gcc/tree.cc > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > > continue; > > > int precision = GET_MODE_PRECISION (mode); > > > - /* Work around the rs6000 KFmode having precision 113 not > > > - 128. */ > > > > It has precision 126 now fwiw. > > > > Joseph: what do you think about this patch? Is the workaround it > > removes still useful in any way, do we need to do that some other way if > > we remove this? > > I think it's best for the TYPE_PRECISION, for any type with the binary128 > format, to be 128 (not 126). Agreed. > It's necessary that _Float128, _Float64x and long double all have the same > TYPE_PRECISION when they have the same (binary128) format, or at least > that TYPE_PRECISION for _Float128 >= that for long double >= that for > _Float64x, so that the rules in c_common_type apply properly. > > How the TYPE_PRECISION compares to that of __ibm128, or of long double > when that's double-double, is less important. I guess it can affect the common type for {long double (when binary128),_Float128,_Float64x,__float128,__ieee128} vs. {long double (when ibm128),__ibm128}, especially in C (for C++ only when non-standard types are involved (__float128, __ieee128, __ibm128). But I think unless we error (e.g. in C++ when we see unordered floating point types), prefering binary128 is better, it certainly has much bigger exponent range over __ibm128 and most of the time also the precision (__ibm128 wastes some bits on the other exponent). Jakub
Hi Segher, on 2022/12/22 05:24, Segher Boessenkool wrote: > Hi! > > On Wed, Dec 21, 2022 at 05:02:17PM +0800, Kewen.Lin wrote: >> This a different attempt from Mike's approach[1][2] to fix the >> issue in PR107299. > > Ke Wen, Mike: so iiuc with this patch applied all three of Mike's > patches are unnecessary? I think the 1st patch is still needed, it's to fix a latent issue as the associated test cases {mul,div}ic3-1.c show. > Does it fix the new testcases in Mike's series as well? Yeah, it doesn't suffer the issue exposed by float128-hw4.c, so it doesn't need that adjustment on float128-hw4.c. It can also make newly added float128-hw{12,13}.c pass. >> As above, I wonder if we can consider this approach which >> makes type _Float128 have the same precision as MODE_PRECISION >> of its mode, it keeps the previous implementation to make type >> long double compatible with _Float128. Since the REAL_MODE_FORMAT >> of the mode still holds the information, even if some place which >> isn't covered in the above testing need the actual precision, we >> can still retrieve the actual precision with that. > > "Precision" does not have a useful meaning for all floating point > formats. It does not have one for double-double for example. The way > precision is defined in IEEE FP means double-double has 2048 bits of > precision (or is it 2047), not useful. Taking the precision of the > format instead to be the minimum over all values in that format gives > double-double the same precision as IEEE DP, not useful in any practical > way either :-/ OK, I think that's why we don't see any regressions with this work around removal, :) > >> --- a/gcc/tree.cc >> +++ b/gcc/tree.cc >> @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) >> if (!targetm.floatn_mode (n, extended).exists (&mode)) >> continue; >> int precision = GET_MODE_PRECISION (mode); >> - /* Work around the rs6000 KFmode having precision 113 not >> - 128. */ > > It has precision 126 now fwiw. Yes, with -mabi=ibmlongdouble, it uses KFmode so 126, while with -mabi=ieeelongdouble, it uses TFmode so 127. BR, Kewen
Hi Joseph, on 2022/12/22 05:40, Joseph Myers wrote: > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > >>> --- a/gcc/tree.cc >>> +++ b/gcc/tree.cc >>> @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) >>> if (!targetm.floatn_mode (n, extended).exists (&mode)) >>> continue; >>> int precision = GET_MODE_PRECISION (mode); >>> - /* Work around the rs6000 KFmode having precision 113 not >>> - 128. */ >> >> It has precision 126 now fwiw. >> >> Joseph: what do you think about this patch? Is the workaround it >> removes still useful in any way, do we need to do that some other way if >> we remove this? > > I think it's best for the TYPE_PRECISION, for any type with the binary128 > format, to be 128 (not 126). I agree that it's more reasonable to use 128 for it (all of them) eventually, but what I thought is that if we can get rid of this workaround first to make the bootstrapping survive. Commit r9-1302-g6a8886e45f7eb6 makes TFmode/ KFmode/IFmode have different precisions with some reasons, Jakub mentioned commit r13-3292, I think later we can refer to it and have a try to unique those modes to have the same precisions (probably next stage1), I guess Mike may have more insightful comments here. > > It's necessary that _Float128, _Float64x and long double all have the same > TYPE_PRECISION when they have the same (binary128) format, or at least > that TYPE_PRECISION for _Float128 >= that for long double >= that for > _Float64x, so that the rules in c_common_type apply properly. > a. _Float128, _Float64x and long double all have the same TYPE_PRECISION when they have the same (binary128) format. b. TYPE_PRECISION for _Float128 >= that for long double >= that for _Float64x. **Without this patch**, 1) -mabi=ieeelongdouble (these three are ieee128) _Float128 => 128 ("type => its TYPE_PRECISION") _Float64x => 128 long double => 127 // Neither a and b holds. 2) -mabi=ibmlongdouble (long double is ibm128) _Float128 => 128 _Float64x => 128 long double => 127 // a N/A, b doesn't hold. **With this patch**, 1) -mabi=ieeelongdouble _Float128 => 127 _Float64x => 127 long double => 127 // both a and b hold. 2) -mabi=ibmlongdouble _Float128 => 126 _Float64x => 126 long double => 127 // a N/A, b doesn't hold. IMHO, this patch improves the status quo slightly. BR, Kewen
Hi! On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > --- a/gcc/tree.cc > > > +++ b/gcc/tree.cc > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > > continue; > > > int precision = GET_MODE_PRECISION (mode); > > > - /* Work around the rs6000 KFmode having precision 113 not > > > - 128. */ > > > > It has precision 126 now fwiw. > > > > Joseph: what do you think about this patch? Is the workaround it > > removes still useful in any way, do we need to do that some other way if > > we remove this? You didn't address these questions. We don't see negative effects from removing this workaround, but it isn't clear (to me) what problems were there that caused you to do this workaround. Do you remember maybe? Or can we just delete it and try to forget such worries :-) > I think it's best for the TYPE_PRECISION, for any type with the binary128 > format, to be 128 (not 126). Well, but why? Of course it looks nicer, and it is a gross workaround to have different precisions for the different 128-bit FP modes, more so if two modes are really the same, but in none of the ways floating point precision is defined would it be 128 for any 128-bit mode. > It's necessary that _Float128, _Float64x and long double all have the same > TYPE_PRECISION when they have the same (binary128) format, Yes, agreed. Or even if it would be not necessary it is the only sane thing to do. Segher
On Thu, 22 Dec 2022, Segher Boessenkool wrote: > Hi! > > On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > > --- a/gcc/tree.cc > > > > +++ b/gcc/tree.cc > > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > > > continue; > > > > int precision = GET_MODE_PRECISION (mode); > > > > - /* Work around the rs6000 KFmode having precision 113 not > > > > - 128. */ > > > > > > It has precision 126 now fwiw. > > > > > > Joseph: what do you think about this patch? Is the workaround it > > > removes still useful in any way, do we need to do that some other way if > > > we remove this? > > You didn't address these questions. We don't see negative effects from > removing this workaround, but it isn't clear (to me) what problems were > there that caused you to do this workaround. Do you remember maybe? Or > can we just delete it and try to forget such worries :-) The purpose was to ensure that _Float128's TYPE_PRECISION was at least as large as that of long double, in the case where they both have binary128 format. I think at that time, in GCC 7, it was possible for _Float128 to be KFmode and long double to be TFmode, with those being different modes with the same format. In my view, it would be best not to have different modes with the same format - not simply ensure types with the same format have the same mode, but avoid multiple modes with the same format existing in the compiler at all. That is, TFmode should be the same mode as one of KFmode and IFmode (one name should be defined as a macro for the other name, or something similar). If you don't have different modes with the same format, many of the problems go away.
On Thu, Dec 22, 2022 at 07:48:28PM +0000, Joseph Myers wrote: > On Thu, 22 Dec 2022, Segher Boessenkool wrote: > > On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > > > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > > Joseph: what do you think about this patch? Is the workaround it > > > > removes still useful in any way, do we need to do that some other way if > > > > we remove this? > > > > You didn't address these questions. We don't see negative effects from > > removing this workaround, but it isn't clear (to me) what problems were > > there that caused you to do this workaround. Do you remember maybe? Or > > can we just delete it and try to forget such worries :-) > > The purpose was to ensure that _Float128's TYPE_PRECISION was at least as > large as that of long double, in the case where they both have binary128 > format. I think at that time, in GCC 7, it was possible for _Float128 to > be KFmode and long double to be TFmode, with those being different modes > with the same format. They still are separate modes :-( It always is possible to create KFmode entities (via mode((KF)) if nothing else) and those should behave exactly the same as TFmode if TFmode is IEEE QP (just like KFmode always is). > In my view, it would be best not to have different modes with the same > format - not simply ensure types with the same format have the same mode, > but avoid multiple modes with the same format existing in the compiler at > all. That is, TFmode should be the same mode as one of KFmode and IFmode > (one name should be defined as a macro for the other name, or something > similar). Right, TFmode should be just a different *name* for either IFmode or KFmode (and both of those modes always exist if either does). > If you don't have different modes with the same format, many of > the problems go away. I used to have patches for this. A few problems remained, but this was very long ago, who knows where we stand now. I'll recreate those patches, let's see where that gets us. Thanks for the help, Segher
On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > > --- a/gcc/tree.cc > > > +++ b/gcc/tree.cc > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > > continue; > > > int precision = GET_MODE_PRECISION (mode); > > > - /* Work around the rs6000 KFmode having precision 113 not > > > - 128. */ > > > > It has precision 126 now fwiw. > > > > Joseph: what do you think about this patch? Is the workaround it > > removes still useful in any way, do we need to do that some other way if > > we remove this? > > I think it's best for the TYPE_PRECISION, for any type with the binary128 > format, to be 128 (not 126). > > It's necessary that _Float128, _Float64x and long double all have the same > TYPE_PRECISION when they have the same (binary128) format, or at least > that TYPE_PRECISION for _Float128 >= that for long double >= that for > _Float64x, so that the rules in c_common_type apply properly. > > How the TYPE_PRECISION compares to that of __ibm128, or of long double > when that's double-double, is less important. When I did the original implementation years ago, there were various implicit assumptions that for any one precision, there must be only one floating point type. I tend to agree that logically the precision should be 128, but until we go through and fix all of these assumptions, it may be problematical. This shows up in the whole infrastructure of looking for a FP type with larger precision than a given precision. There just isn't an ordering that works and preserves all values. I'm coming to think that we may want 2 types of FP, one is a standard FP type where you can convert to a larger type, and the other for various FP types where there is no default widening conversion. And logically there is the issue with 16-bit floats, giving we have different versions of 16-bit float. And if an implementation ever wanted to support both BID and DFP decimal types at the same time, they would have similar issues.
On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > > --- a/gcc/tree.cc > > > +++ b/gcc/tree.cc > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > > continue; > > > int precision = GET_MODE_PRECISION (mode); > > > - /* Work around the rs6000 KFmode having precision 113 not > > > - 128. */ > > > > It has precision 126 now fwiw. > > > > Joseph: what do you think about this patch? Is the workaround it > > removes still useful in any way, do we need to do that some other way if > > we remove this? > > I think it's best for the TYPE_PRECISION, for any type with the binary128 > format, to be 128 (not 126). > > It's necessary that _Float128, _Float64x and long double all have the same > TYPE_PRECISION when they have the same (binary128) format, or at least > that TYPE_PRECISION for _Float128 >= that for long double >= that for > _Float64x, so that the rules in c_common_type apply properly. > > How the TYPE_PRECISION compares to that of __ibm128, or of long double > when that's double-double, is less important. I spent a few days on working on this. I have patches to make the 3 128-bit types to all have TYPE_PRECISION of 128. To do this, I added a new mode macro (FRACTIONAL_FLOAT_MODE_NO_WIDEN) that takes the same arguments as FRACTIONAL_FLOAT_MODE. This will create a floating point mode that is a normal floating point mode, but the GET_MODE_WIDER and GET_MODE_2XWIDER macros will never return it. By declaring both IFmode and KFmode to not be widened to, but noral TFmode is, it eliminates the problems where an IBM expression got converted to IEEE, which can mostly (but not always) contain the value. In addition, on power8, it means it won't call the KF mode emulator functions, just the IF functions. We need to have one 128-bit mode (TFmode) that is not declared as NO_WARN, or long double won't be created, since float_mode_for_size (128) will not be able to find the correct type. I did have to patch convert_mode_scalar so that it would not abort if it was doing a conversion between two floating point types with the same precision. I tested this with the first patch from the previous set of patches (that rewrites complex multiply/divide built-in setup). I think that patch is useful as a stand alone patch. I also used Kewen Lin's patch from December 27th in build_common_tree_nodes to do the test. I haven't tested if this particular patch fixes this problem, or it fixes something else. Finally, I used the third patch in my series of patches that straightens out 128<->128 FP conversions. That patch needed to be tweaked slightly, as one of the conversations became FLOAT_EXTEND instead of FLOAT_TRUNCATE. We don't have a RTL operation that says convert from one floating point type to another where both types are the same size. Whether FLOAT_EXTEND is used or FLOAT_TRUNCATE, used to depend on whether the TYPE_PRECISION was greater or lesser. Now that they are the same, it arbitrarily picks FLOAT_EXTEND. While I still think the 2nd patch is important, it isn't needed with the above patches.
On Fri, Jan 06, 2023 at 07:41:07PM -0500, Michael Meissner wrote: > On Wed, Dec 21, 2022 at 09:40:24PM +0000, Joseph Myers wrote: > > On Wed, 21 Dec 2022, Segher Boessenkool wrote: > > > > > > --- a/gcc/tree.cc > > > > +++ b/gcc/tree.cc > > > > @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) > > > > if (!targetm.floatn_mode (n, extended).exists (&mode)) > > > > continue; > > > > int precision = GET_MODE_PRECISION (mode); > > > > - /* Work around the rs6000 KFmode having precision 113 not > > > > - 128. */ > > > > > > It has precision 126 now fwiw. > > > > > > Joseph: what do you think about this patch? Is the workaround it > > > removes still useful in any way, do we need to do that some other way if > > > we remove this? > > > > I think it's best for the TYPE_PRECISION, for any type with the binary128 > > format, to be 128 (not 126). > > > > It's necessary that _Float128, _Float64x and long double all have the same > > TYPE_PRECISION when they have the same (binary128) format, or at least > > that TYPE_PRECISION for _Float128 >= that for long double >= that for > > _Float64x, so that the rules in c_common_type apply properly. > > > > How the TYPE_PRECISION compares to that of __ibm128, or of long double > > when that's double-double, is less important. > > I spent a few days on working on this. I have patches to make the 3 128-bit > types to all have TYPE_PRECISION of 128. To do this, I added a new mode macro > (FRACTIONAL_FLOAT_MODE_NO_WIDEN) that takes the same arguments as > FRACTIONAL_FLOAT_MODE. ... I had the patches to change the precision to 128, and I just ran them. C and C++ do not seem to be bothered by changing the precision to 128 (once I got it to build, etc.). But Fortran on the other hand does actually use the precision to differentiate between IBM extended double and IEEE 128-bit. In particular, the following 3 tests fail when long double is IBM extended double: gfortran.dg/PR100914.f90 gfortran.dg/c-interop/typecodes-array-float128.f90 gfortran.dg/c-interop/typecodes-scalar-float128.f90 I tried adding code to use the old precisions for Fortran, but not for C/C++, but it didn't seem to work. So while it might be possible to use a single 128 for the precision, it needs more work and attention, particularly on the Fortran side. I'm not sure it is worth it to try and change things.
On Mon, Jan 09, 2023 at 10:21:52PM -0500, Michael Meissner wrote: > I had the patches to change the precision to 128, and I just ran them. C and > C++ do not seem to be bothered by changing the precision to 128 (once I got it > to build, etc.). But Fortran on the other hand does actually use the precision > to differentiate between IBM extended double and IEEE 128-bit. In particular, > the following 3 tests fail when long double is IBM extended double: > > gfortran.dg/PR100914.f90 > gfortran.dg/c-interop/typecodes-array-float128.f90 > gfortran.dg/c-interop/typecodes-scalar-float128.f90 > > I tried adding code to use the old precisions for Fortran, but not for C/C++, > but it didn't seem to work. > > So while it might be possible to use a single 128 for the precision, it needs > more work and attention, particularly on the Fortran side. Can't be more than a few lines changed in the fortran FE. Yes, the FE needs to know if it is IBM extended double or IEEE 128-bit so that it can decide on the mangling - where to use the artificial kind 17 and where to use 16. But as long as it can figure that out, it doesn't need to rely on a particular precision. Jakub
On Tue, Jan 10, 2023 at 07:23:23PM +0100, Jakub Jelinek wrote: > On Mon, Jan 09, 2023 at 10:21:52PM -0500, Michael Meissner wrote: > > I had the patches to change the precision to 128, and I just ran them. C and > > C++ do not seem to be bothered by changing the precision to 128 (once I got it > > to build, etc.). But Fortran on the other hand does actually use the precision > > to differentiate between IBM extended double and IEEE 128-bit. In particular, > > the following 3 tests fail when long double is IBM extended double: > > > > gfortran.dg/PR100914.f90 > > gfortran.dg/c-interop/typecodes-array-float128.f90 > > gfortran.dg/c-interop/typecodes-scalar-float128.f90 > > > > I tried adding code to use the old precisions for Fortran, but not for C/C++, > > but it didn't seem to work. > > > > So while it might be possible to use a single 128 for the precision, it needs > > more work and attention, particularly on the Fortran side. > > Can't be more than a few lines changed in the fortran FE. > Yes, the FE needs to know if it is IBM extended double or IEEE 128-bit so > that it can decide on the mangling - where to use the artificial kind 17 and > where to use 16. But as long as it can figure that out, it doesn't need to > rely on a particular precision. I agree that in theory it should be simple to fix. Unfortunately the patches that I was working on cause some other failures that I need to investigate.
diff --git a/gcc/tree.cc b/gcc/tree.cc index 254b2373dcf..cbae14d095e 100644 --- a/gcc/tree.cc +++ b/gcc/tree.cc @@ -9442,15 +9442,6 @@ build_common_tree_nodes (bool signed_char) if (!targetm.floatn_mode (n, extended).exists (&mode)) continue; int precision = GET_MODE_PRECISION (mode); - /* Work around the rs6000 KFmode having precision 113 not - 128. */ - const struct real_format *fmt = REAL_MODE_FORMAT (mode); - gcc_assert (fmt->b == 2 && fmt->emin + fmt->emax == 3); - int min_precision = fmt->p + ceil_log2 (fmt->emax - fmt->emin); - if (!extended) - gcc_assert (min_precision == n); - if (precision < min_precision) - precision = min_precision; FLOATN_NX_TYPE_NODE (i) = make_node (REAL_TYPE); TYPE_PRECISION (FLOATN_NX_TYPE_NODE (i)) = precision; layout_type (FLOATN_NX_TYPE_NODE (i));