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 4FE823858D35 for ; Thu, 25 Nov 2021 21:13:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4FE823858D35 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 1APLCYlS002272; Thu, 25 Nov 2021 15:12:34 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 1APLCWwV002269; Thu, 25 Nov 2021 15:12:32 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 25 Nov 2021 15:12:32 -0600 From: Segher Boessenkool To: 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: <20211125211232.GO614@gate.crashing.org> References: <20211124234847.tw7xh6pldu5me3mv@work-tp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211124234847.tw7xh6pldu5me3mv@work-tp> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_PASS, TXREP 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: Thu, 25 Nov 2021 21:13:41 -0000 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?) > * 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. > * doc/md.texi: (fegetround@var{m}): Document new optab. > (feclearexcept@var{m}): Document new optab. > (feraiseexcept@var{m}): Document new optab. > * optabs.def (fegetround_optab): New optab. > (feclearexcept_optab): New optab. > (feraiseexcept_optab): New optab. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: New test. > * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: New test. > * gcc.target/powerpc/builtin-fegetround.c: New test. > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -6860,6 +6860,117 @@ > [(set_attr "type" "fpload") > (set_attr "length" "8") > (set_attr "isa" "*,p8v,p8v")]) > + > +;; 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? > +;; 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 :-) > +;; 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/ > +;; this expand only expands for target libcs that it can handle the (this expander) > +;; values is knows. s/is/it/ > +/* 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. This is a testcase so anything goes, but FYI :-) > --- /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. Maybe test more values? At least 0, but also combinations of these FE_ bits, and maybe even FE_INVALID? 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