Re: [PATCH] S390: Refactor ifunc handling

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 03/12/2018 06:48, Stefan Liebler wrote:
> On 11/30/2018 07:31 PM, Adhemerval Zanella wrote:
>>
>>
>> On 30/11/2018 13:57, Stefan Liebler wrote:
>>> This patch series is mainly refactoring the s390 specific ifunc handling.
>>> The idea is to omit ifunc variants or ifunc at all if the used compile
>>> options are already building for newer cpus. The glibc internal calls
>>> and ld.so will then use "newer" ifunc variants as before.
>>>
>>> In case of the memcpy, memset and memcmp functions, the newest ifunc variant
>>> is for z196 and there are two further ones for older cpus, but the current usual
>>> compile options are e.g. building for zEC12.
>>> In case of the string / wcsmbs functions, there are variants for z13 and
>>> "before z13".  After switching to z13 as default cpu level, there won't
>>> be IFUNC symbols in s390 libc.so.
>>>
>>> Furthermore new z13 specific ifunc variants are introduced for
>>> memmove, strstr and memmem.
>>>
>>> Some functions like the mem* functions are duplicated twice for 31 and 64 bit.
>>> In fact they are nearly the same. Thus those implementations are now unified
>>> and adjusted in order to be usable for 31 and 64bit.
>>>
>>> I've build and tested these patches with different -march levels
>>> and with / without multiarch and checked the symbols with readelf - e.g. if
>>> IFUNC is used or not and if the __GI_ symbols are targeting the correct
>>> ifunc variant.
>>>
>>> If no one objects, I plan to commit this series in the next one or two weeks.
>>>
>>
>> The only issue I have for this change is it would require another
>> ABI variant for testing and validation, which would require more
>> coverage from build-many-glibcs.py (similar to armv7 for instance).
> The ABI itself is not changed, but you are right tests could be done for e.g. -march=zEC12 and -march=z13

I meant 'variant' in the sense it would require multiple glibc build to
actually validate a change in s390 implementations.  If I am reading it
correctly, on memcpy, for instance, we might have:

  1. HAVE_MEMCPY_IFUNC 
    1.1. HAVE_S390_MIN_Z196_ZARCH_ASM_SUPPORT
    1.2. HAVE_S390_MIN_Z10_ZARCH_ASM_SUPPORT
    1.3. Z900
  2. !HAVE_MEMCPY_IFUNC
  3. Disabled multi-arch

So is is basically 4 different glibc builds one will need to actually
test and check to fully validate s390.

> 
>>
>> The gains I see is a slight reduction is loading time (due no ifunc)
>> and less code side. Is is what is driving you for this change? Does
>> it worth the extra testing and validation it might require?
>>
> The current s390_vx_libc_ifunc macros are not flexible and do not allow future enhancements as they only use the *_c or *_vx variant.

Right, this is one issue which is not really tied to setting up different
build variants depending of the minimum ISA level.

> 
> As soon as future ditros will use z13 as default architecture,
> vector instructions can be used by default.  But the current __GI_* functions won't use the vector variants.  Instead the fallback is always used for internal calls and the vector variants are always used for external calls via IFUNC.

Some ABI does impose restriction for ifunc with internal hidden symbols
(powerpc32 and i686 if I recall correctly), is it the case for s390 
and/or s390x?

At least for some ABI, we can still route some internal symbols through
ifunc. On some x86 ifunc selector implementation the comments state it
might show no performance gain, but I am not sure about the validity of
the claims. So if s390 or s390x does not have any impeding reason, a
possibility might to use ifunc internally instead of relying on compiler
default or used options to setup the minimum ABI.

So the question is whether the possible internal symbols call optimization
worth that added complexity for build and validation.