From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa2.mentor.iphmx.com (esa2.mentor.iphmx.com [68.232.141.98]) by sourceware.org (Postfix) with ESMTPS id 2CA0D385780D for ; Tue, 5 Apr 2022 15:54:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2CA0D385780D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.90,236,1643702400"; d="scan'208";a="74102403" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa2.mentor.iphmx.com with ESMTP; 05 Apr 2022 07:53:59 -0800 IronPort-SDR: jC7aLc765TIJboIPxwTdQcljXQ5xLS702QHJVQ2MU5Q2nsM9ay/oJ/S6ZtUhNm9nYUMt5nwzGb fet1sHC7+s9YerzV/MynTvAULLO0SI4MFumUVp+d7EVTTJ8r1ql7pwN3wH5Pokig2Ao7RvtNkX CX6DuPVy9bVqj/4anfRHCA6y80fv02H0DUNC4OHbNahryHTloKgftf7UqOfrJJsf36N4veDQlL N755Crr1VSO2ONk5Y7+QX2ahqN6I8r7es2UI8clBa962EtA1QI5Zlj99eDHNd70l9UEHsw21p0 rH8= Date: Tue, 5 Apr 2022 15:53:54 +0000 From: Joseph Myers X-X-Sender: jsm28@digraph.polyomino.org.uk To: Adhemerval Zanella CC: Subject: Re: [PATCH] math: Add math-use-builtins-fabs In-Reply-To: <4770fe55-59aa-6a25-dd3a-bc1b589b4270@linaro.org> Message-ID: References: <20220404205805.701759-1-adhemerval.zanella@linaro.org> <06894f7e-df86-9ed0-8a61-d4077f920e9d@linaro.org> <4770fe55-59aa-6a25-dd3a-bc1b589b4270@linaro.org> User-Agent: Alpine 2.22 (DEB 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-06.mgc.mentorg.com (139.181.222.6) To svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) X-Spam-Status: No, score=-3114.0 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, 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: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Apr 2022 15:54:02 -0000 On Tue, 5 Apr 2022, Adhemerval Zanella via Libc-alpha wrote: > On 04/04/2022 22:55, Adhemerval Zanella wrote: > > > > > > On 04/04/2022 19:41, Joseph Myers wrote: > >> On Mon, 4 Apr 2022, Adhemerval Zanella via Libc-alpha wrote: > >> > >>> to support building glibc with clang, which does not support inline > >>> __builtin_fabsf128. I change the patch to assume _Float128 support, > >> > >> And also not under another name (cf. how sysdeps/x86/bits/floatn.h uses > >> __builtin_fabsq before GCC 7)? > >> > > > > It does not seems to, it issues an undefined builtin error. > > So do you prefer I just assume long double and _Float128 abs builtin > support or the patch is good as is? I think there are a few issues here: 1. If something is more conveniently implemented using a GCC feature (supported in all GCC versions supported for building glibc), you need a justification for why it's appropriate to complicate the glibc code with conditionals for compilers not supporting that feature, versus saying that building with clang would require a minimum version of clang that supports that feature. I wouldn't expect __builtin_fabsf128 to be a problematic feature to add to clang (and likewise any other _Float128 built-in functions needed, on platforms where glibc supports _Float128 with a different format from long double) - so the starting point should be to require a clang version with that feature, and doing otherwise needs a specific justification. And *any* case of working around the absence of a feature needs case-by-case justification for doing so. This is different from the case where avoiding the feature also results in cleaner code in glibc, not in adding any conditionals - which has typically been the case for changes to avoid nested functions by making interfaces pass relevant state more explicitly, for example. 2. Where code in installed headers is involved, conditionals *are* appropriate - and that includes conditionals on different clang versions, to support older versions before whatever feature was added to clang. *But* while bits/floatn.h is an installed header, the conditional __builtin_fabsf128 definition there is only actually relevant when building glibc - nothing else in the installed headers uses __builtin_fabsf128 - so version conditionals for clang versions too old to build glibc aren't needed for it. (This is different from the __builtin_signbitf128 definition in bits/floatn.h where __builtin_signbitf128 *is* used in installed math/math.h - though in that case, there's already a __glibc_clang_prereq (3,3) conditional in math/math.h.) 3. Given a requirement for a clang version supporting __builtin_fabsf128 to build glibc on relevant platforms, built-in fabs conditionals should only be needed for the case of IBM long double for soft-float powerpc; no other case should need to have such a conditional or to include math-use-builtins-fabs.h at all, and only two copies of math-use-builtins-fabs.h should be needed. -- Joseph S. Myers joseph@codesourcery.com