Re: [PATCH] math: Add math-use-builtins-fabs

[Joseph Myers <joseph@codesourcery.com>]

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