From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 705933858418 for ; Thu, 23 Dec 2021 17:16:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 705933858418 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 1BNH7Obl013711; Thu, 23 Dec 2021 17:16:51 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3d4uhua9k7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 23 Dec 2021 17:16:50 +0000 Received: from m0098416.ppops.net (m0098416.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 1BNH7dc3013967; Thu, 23 Dec 2021 17:16:50 GMT Received: from ppma04dal.us.ibm.com (7a.29.35a9.ip4.static.sl-reverse.com [169.53.41.122]) by mx0b-001b2d01.pphosted.com with ESMTP id 3d4uhua9jx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 23 Dec 2021 17:16:50 +0000 Received: from pps.filterd (ppma04dal.us.ibm.com [127.0.0.1]) by ppma04dal.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 1BNGuqT7019833; Thu, 23 Dec 2021 17:16:49 GMT Received: from b01cxnp22033.gho.pok.ibm.com (b01cxnp22033.gho.pok.ibm.com [9.57.198.23]) by ppma04dal.us.ibm.com with ESMTP id 3d416u9fxb-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 23 Dec 2021 17:16:49 +0000 Received: from b01ledav005.gho.pok.ibm.com (b01ledav005.gho.pok.ibm.com [9.57.199.110]) by b01cxnp22033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 1BNHGmNO29426172 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 23 Dec 2021 17:16:48 GMT Received: from b01ledav005.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A66BDAE05C; Thu, 23 Dec 2021 17:16:48 +0000 (GMT) Received: from b01ledav005.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D65C0AE062; Thu, 23 Dec 2021 17:16:46 +0000 (GMT) Received: from work-tp (unknown [9.65.243.99]) by b01ledav005.gho.pok.ibm.com (Postfix) with ESMTPS; Thu, 23 Dec 2021 17:16:46 +0000 (GMT) Date: Thu, 23 Dec 2021 14:16:43 -0300 From: Raoni Fassina Firmino To: Segher Boessenkool Cc: gcc-patches@gcc.gnu.org, joseph@codesourcery.com, jakub@redhat.com, rguenther@suse.de, hp@bitrange.com, law@redhat.com, will_schmidt@vnet.ibm.com Subject: Re: [PATCH v7] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193] Message-ID: <20211223171643.gaaspvaqcr7lwdg3@work-tp> Mail-Followup-To: Segher Boessenkool , gcc-patches@gcc.gnu.org, joseph@codesourcery.com, jakub@redhat.com, rguenther@suse.de, hp@bitrange.com, law@redhat.com, will_schmidt@vnet.ibm.com References: <20211124234847.tw7xh6pldu5me3mv@work-tp> <20211125211232.GO614@gate.crashing.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211125211232.GO614@gate.crashing.org> X-TM-AS-GCONF: 00 X-Proofpoint-GUID: CD1HIBJ1gOc7xeFhuFTta3-Rv15GOPJS X-Proofpoint-ORIG-GUID: XUi1mC7zm856N0NXzF_54iHqj0_jDCht X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2021-12-23_04,2021-12-22_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 adultscore=0 priorityscore=1501 impostorscore=0 phishscore=0 mlxlogscore=999 lowpriorityscore=0 bulkscore=0 mlxscore=0 clxscore=1015 spamscore=0 suspectscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2112230089 X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP 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: Thu, 23 Dec 2021 17:16:56 -0000 Hi Segher, On Thu, Nov 25, 2021 at 03:12:32PM -0600, Segher Boessenkool wrote: > Hi! > > On Wed, Nov 24, 2021 at 08:48:47PM -0300, Raoni Fassina Firmino wrote: > > gcc/ChangeLog: > > * builtins.c (expand_builtin_fegetround): New function. > > (expand_builtin_feclear_feraise_except): New function. > > (expand_builtin): Add cases for BUILT_IN_FEGETROUND, > > BUILT_IN_FECLEAREXCEPT and BUILT_IN_FERAISEEXCEPT > > Something is missing here (maybe just a full stop?) Yeap. Done. > > * config/rs6000/rs6000.md (fegetroundsi): New pattern. > > (feclearexceptsi): New Pattern. > > (feraiseexceptsi): New Pattern. > > * doc/extend.texi: Add a new introductory paragraph about the > > new builtins. > > Pet peeve: please don't break lines early, we have only 72 columns per > line and we have many long symbol names. Trying to make many lines very > short only results in everything looking very irregular, which is harder > to read. Sure thing, it is my bad that I have shortcuts for 70 and 80 textwidth but not 72. In any case: | * doc/extend.texi: Add a new introductory paragraph about the new | builtins. It would be 73 columns or I am reading my text editor wrong? Also here: |gcc/testsuite/ChangeLog: | | * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: New test. | * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: New test. It is 79 for now, but has the same 73 problem, I guess the correct formatting is: |gcc/testsuite/ChangeLog: | | * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: | New test. | * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: | New test. is that right? > > +;; int fegetround(void) > > +;; > > +;; This expansion for the C99 function only expands for compatible > > +;; target libcs. Because it needs to return one of FE_DOWNWARD, > > +;; FE_TONEAREST, FE_TOWARDZERO or FE_UPWARD with the values as defined > > +;; by the target libc, and since they are free to > > +;; choose the values and the expand needs to know then beforehand, > > +;; this expand only expands for target libcs that it can handle the > > +;; values is knows. > > +;; Because of these restriction, this only expands on the desired > > +;; case and fallback to a call to libc on any otherwise. > > +(define_expand "fegetroundsi" > > (This needs some wordsmithing.) > > +;; int feclearexcept(int excepts) > > +;; > > +;; This expansion for the C99 function only works when EXCEPTS is a > > +;; constant known at compile time and specifies any one of > > +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags. > > +;; It doesn't handle values out of range, and always returns 0. > > It FAILs the expansion if a parameter is bad? Is this comment out of > date? If the parameter is one that it cannot handle, including boggus values, it then FAILs and the libc function will handle it, including returning error for wrong input. This part is verbatin from v5 (priour to the refactoring that then was undone) > > +;; Note that FE_INVALID is unsupported because it maps to more than > > +;; one bit of the FPSCR register. > > It could be implemented, now that you check for the libc used. It is a > fixed part of the ABI :-) Oh yeah, I can add it now or in a subsequent commit, is that a hard requirement for the patch? > > +;; The FE_* are defined in the targed libc, and since they are free to > > +;; choose the values and the expand needs to know then beforehand, > > s/then/them/ Done. > > +;; this expand only expands for target libcs that it can handle the > > (this expander) Done. > > +;; values is knows. > > s/is/it/ Done. > > +/* This testcase ensures that the builtins expand with the matching arguments > > + * or otherwise fallback gracefully to a function call, and don't ICE during > > + * compilation. > > + * "-fno-builtin" option is used to enable calls to libc implementation of the > > + * gcc builtins tested when not using __builtin_ prefix. */ > > Don't use leading * in comments, btw. Done. > This is a testcase so anything > goes, but FYI :-) Yeah, better keep the same style :-) > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c > > > + int i, rounding, expected; > > + const int rm[] = {FE_TONEAREST, FE_TOWARDZERO, FE_UPWARD, FE_DOWNWARD}; > > + for (i = 0; i < sizeof(rm); i++) > > That should be sizeof rm / sizeof rm[0] ? It accesses out of bounds > as it is. Done. Thanks for the catch, newbie mistake on my part. > Maybe test more values? At least 0, but also combinations of these FE_ > bits, and maybe even FE_INVALID? I Don't get what you mean, like use some invalid values for fesetround()? I am using only expected values because fegetround() will only read what was previously set. I could set some invalid values and expect that it did not change the value expected to be read in fegetround but then I would be testing fesetround and not fegetround I guess. > > With such changes the rs6000 parts are okay for trunk. Thanks! > > I looked at the generic changes as well, and they all look fine to me. > > > Segher Thanks :) o/ Raoni