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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 05/04/2022 12:53, Joseph Myers wrote:
> 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.

The issue even main branch does not support __builtin_fabs128 without routing
it to a libcall, so requiring a minimum clang version might not make much
sense.  I might make it work for next 15 version, but since it is not yet
release I think a workaround in glibc is justifiable. 

> 
> 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.

The idea is indeed simplify the code, so I think it would be better to just
assume gcc-only support and add the possible required clang changes in the
patchset itself.

> 
> 
> 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.
>