[PATCH 4/4] S390: Implement mempcpy with help of memcpy. [BZ #19765]
Stefan Liebler
stli@linux.vnet.ibm.com
Wed May 18 15:25:00 GMT 2016
On 05/13/2016 05:30 PM, Stefan Liebler wrote:
>
>
> On 05/13/2016 05:06 PM, H.J. Lu wrote:
>> On Fri, May 13, 2016 at 7:59 AM, Stefan Liebler
>> <stli@linux.vnet.ibm.com> wrote:
>>>
>>>
>>> On 05/13/2016 04:49 PM, H.J. Lu wrote:
>>>>
>>>> On Fri, May 13, 2016 at 7:42 AM, Stefan Liebler
>>>> <stli@linux.vnet.ibm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 05/12/2016 04:10 PM, Adhemerval Zanella wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 09/05/2016 11:15, Stefan Liebler wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 05/05/2016 06:36 PM, H.J. Lu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, May 5, 2016 at 9:34 AM, Adhemerval Zanella
>>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On May 5, 2016, at 11:45, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Thu, May 5, 2016 at 7:15 AM, Adhemerval Zanella
>>>>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> On 05/05/2016 10:37, H.J. Lu wrote:
>>>>>>>>>>>> On Wed, May 4, 2016 at 1:58 PM, Adhemerval Zanella
>>>>>>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 04/05/2016 17:51, Wilco Dijkstra wrote:
>>>>>>>>>>>>>> Adhemerval Zanella wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But my point is all the architectures which provide an
>>>>>>>>>>>>>>> optimized
>>>>>>>>>>>>>>> mempcpy is
>>>>>>>>>>>>>>> though either 1. jump directly to optimized memcpy (s390
>>>>>>>>>>>>>>> case
>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>> this patchset),
>>>>>>>>>>>>>>> 2. clonning the same memcpy implementation and adjusting the
>>>>>>>>>>>>>>> pointers (x86_64) or
>>>>>>>>>>>>>>> 3. using a similar strategy for both implementations
>>>>>>>>>>>>>>> (powerpc).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Indeed, which of those are used doesn't matter much.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So for this change I am proposing compiler support won't be
>>>>>>>>>>>>>>> required because both
>>>>>>>>>>>>>>> memcpy and __mempcpy will be transformed to memcpy + s.
>>>>>>>>>>>>>>> Based
>>>>>>>>>>>>>>> on
>>>>>>>>>>>>>>> assumption that
>>>>>>>>>>>>>>> memcpy is fast as mempcpy implementation I think there is no
>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>> to just add
>>>>>>>>>>>>>>> this micro-optimization to only s390, but rather make is
>>>>>>>>>>>>>>> general.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> GLIBC already has this optimization in the generic string
>>>>>>>>>>>>>> header,
>>>>>>>>>>>>>> it's just that s390 wants
>>>>>>>>>>>>>> to do something different again. As long as GCC isn't
>>>>>>>>>>>>>> fixed this
>>>>>>>>>>>>>> isn't possible to support
>>>>>>>>>>>>>> s390 without this header workaround. And we need GCC to
>>>>>>>>>>>>>> improve
>>>>>>>>>>>>>> so
>>>>>>>>>>>>>> things work
>>>>>>>>>>>>>> better for all the other C libraries...
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> But the current one at string/string.h is only enabled with
>>>>>>>>>>>>> !defined _HAVE_STRING_ARCH_mempcpy,
>>>>>>>>>>>>> so if a port actually adds a mempcpy one it won't be enabled.
>>>>>>>>>>>>> What
>>>>>>>>>>>>> I am trying to argue it
>>>>>>>>>>>>> to just remove the !defined _HAVE_STRING_ARCH_mempcpy and
>>>>>>>>>>>>> enable
>>>>>>>>>>>>> it
>>>>>>>>>>>>> as default for all
>>>>>>>>>>>>> ports.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Please don't enable it for x86. Calling memcpy means we
>>>>>>>>>>>> have to
>>>>>>>>>>>> save and restore 2 registers for no good reasons.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yes, direct call will require save and restore the size for
>>>>>>>>>>> further
>>>>>>>>>>> add
>>>>>>>>>>> and this is true for most architectures. My question is if does
>>>>>>>>>>> this
>>>>>>>>>>> really matter in currently GLIBC internal usage and on programs
>>>>>>>>>>> that
>>>>>>>>>>> might use it compared against the burden of keeping the various
>>>>>>>>>>> string*.h header in check for multiple architectures or
>>>>>>>>>>> adding this
>>>>>>>>>>> logic (mempcpy transformation to memcpy) on compiler.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> What burden? There is nothing to do in glibc for x86. GCC can
>>>>>>>>>> inline mempcpy for x86.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> In fact I am objecting all the bits GLIBC added on string*.h that
>>>>>>>>> only
>>>>>>>>> adds complexity for some micro-optimizations. For x86 I do
>>>>>>>>> agree that
>>>>>>>>> transforming mempcpy to memcpy is no the best strategy.
>>>>>>>>>
>>>>>>>>> My rationale is to avoid add even more arch-specific bits in
>>>>>>>>> installed
>>>>>>>>> headers to add such optimizations.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I believe most of those micro-optimizations belong to GCC, not
>>>>>>>> glibc.
>>>>>>>> Of course, we should keep the existing ones for older GCCs. We
>>>>>>>> should avoid adding new ones.
>>>>>>>>
>>>>>>>
>>>>>>> Does this mean, the proposed way is to not add a macro for
>>>>>>> mempcpy in
>>>>>>> sysdeps/s390/bits/string.h or e.g. for another architecture?
>>>>>>>
>>>>>>> If _HAVE_STRING_ARCH_mempcpy is not defined for s390, then it will
>>>>>>> always
>>>>>>> use the macro defined in string/string.h, which inlines memcpy +
>>>>>>> n and
>>>>>>> the
>>>>>>> mempcpy-function is not called. The memcpy is transformed to mvc,
>>>>>>> mvhi
>>>>>>> for
>>>>>>> e.g. constant lengths. Otherwise, the memcpy-function is called
>>>>>>> and the
>>>>>>> length will be added after the call.
>>>>>>>
>>>>>>> According to PR70140
>>>>>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70140), the macro in
>>>>>>> string/string.h will be removed after this bug is fixed in GCC?
>>>>>>> Then I think the decision, if mempcpy or memcpy is called or an
>>>>>>> inline
>>>>>>> version is emitted, will be done per architecture backend?
>>>>>>>
>>>>>>> If this is all true, then the mempcpy-function will be called in
>>>>>>> future
>>>>>>> if it makes sense!?
>>>>>>>
>>>>>>>
>>>>>>> If _HAVE_STRING_ARCH_mempcpy will be defined for s390, then
>>>>>>> mempcpy-function is always called - even for cases with constant
>>>>>>> length
>>>>>>> where mvc or mvhi is emitted.
>>>>>>> This is definitely not the intention of this patch.
>>>>>>> After fixing PR70140, GCC will correctly handle the constant length
>>>>>>> cases!?
>>>>>>
>>>>>>
>>>>>>
>>>>>> What I am proposing is to avoid add more arch-specific
>>>>>> optimization on
>>>>>> installed
>>>>>> string*.h headers and instead work on adding them on compiler
>>>>>> side. My
>>>>>> view is
>>>>>> we should cleanup as much as possible the string headers and only add
>>>>>> optimization
>>>>>> that are more architecture neutral.
>>>>>>
>>>>>> Related to patch, my understanding is s390x does not really
>>>>>> provide an
>>>>>> optimized
>>>>>> mempcpy (it uses the default mempcpy.c) and I think a better approach
>>>>>> would be
>>>>>> to add an optimized mempcpy like x88_64
>>>>>> (c365e615f7429aee302f8af7bf07ae262278febb).
>>>>>> The mempcpy won't be optimized directly to mvc instruction, but at
>>>>>> least
>>>>>> it will
>>>>>> call the optimized memcpy. The full optimization will be done by
>>>>>> correctly
>>>>>> handling the transformation on compiler size (the PR#70140 as you
>>>>>> referred).
>>>>>>
>>>>>>
>>>>>
>>>>> Okay. Here is the updated patch. It implements mempcpy with memcpy as
>>>>> before, but does not change the s390-specific string.h and
>>>>> _HAVE_STRING_ARCH_mempcpy is not defined. Thus at the moment mempcpy()
>>>>> won't
>>>>> be called, but instead memcpy + n is inlined by the mempcpy macro in
>>>>> string/string.h. After fxing PR70140, either the mempcpy macro in
>>>>> string/string.h has to be removed or _HAVE_STRING_ARCH_mempcpy has
>>>>> to be
>>>>> defined for S390.
>>>>>
>>>>> Okay to commit?
>>>>
>>>>
>>>> +__asm__ (".weak mempcpy\n\t"
>>>> + ".set mempcpy,__mempcpy\n\t");
>>>>
>>>> Don't we have a macro for this?
>>>>
>>> Yes that is true, but I can't use it in mempcpy.c in this case,
>>> because the macro s390_libc_ifunc (__mempcpy) uses an inline assembly
>>> to implement the __mempcpy ifunc symbol.
>>> If I use the weak_alias macro here in the c-file, gcc fails because
>>> it has
>>> no knowledge of symbol __mempcpy.
>>>
>>
>> Can you make s390_libc_ifunc similar to libc_ifunc to expose
>> FUNC to gcc?
>>
>
> No I won't do that.
> At the moment s390_vx_libc_ifunc is implemented in this way,
> then the weak_alias works.
> But I have to remove the asm(FUNCNAME) in
> "extern void *resolverfunction(..) asm (FUNCNAME)"!
>
> The dwarf information for the resolverfunction has a DW_AT_linkage_name
> entry, which shows to FUNCNAME. If you do an inferior function call to
> FUNCNAME in lldb, then it fails due to something like that:
> "error: no matching function for call to 'FUNCNAME'
> candidate function not viable: no known conversion from 'const char [6]'
> to 'unsigned long' for 1st argument"
> (The unsigned long is the dl_hwcap argument of the resolverfunction).
>
> Here is the dwarf information for e.g. __resolve___strlen:
> <1><1e6424>: Abbrev Number: 43 (DW_TAG_subprogram)
> <1e6425> DW_AT_external : 1
> <1e6425> DW_AT_name : (indirect string, offset: 0x1146e):
> __resolve___strlen
> <1e6429> DW_AT_decl_file : 1
> <1e642a> DW_AT_decl_line : 23
> <1e642b> DW_AT_linkage_name: (indirect string, offset: 0x1147a):
> strlen
> <1e642f> DW_AT_prototyped : 1
> <1e642f> DW_AT_type : <0x1e4ccd>
> <1e6433> DW_AT_low_pc : 0x998e0
> <1e643b> DW_AT_high_pc : 0x16
> <1e6443> DW_AT_frame_base : 1 byte block: 9c
> (DW_OP_call_frame_cfa)
> <1e6445> DW_AT_GNU_all_call_sites: 1
> <1e6445> DW_AT_sibling : <0x1e6459>
> <2><1e6449>: Abbrev Number: 44 (DW_TAG_formal_parameter)
> <1e644a> DW_AT_name : (indirect string, offset: 0x1845):
> dl_hwcap
> <1e644e> DW_AT_decl_file : 1
> <1e644f> DW_AT_decl_line : 23
> <1e6450> DW_AT_type : <0x1e4c8d>
> <1e6454> DW_AT_location : 0x122115 (location list)
>
>
> Without the asm (FUNCNAME), there is no DW_AT_linkage_name entry and the
> inferior function call is working.
>
> Do you have any better idea?
>
>
Any objection?
Otherwise I'll commit the patch series soon.
Bye
Stefan
More information about the Libc-alpha
mailing list