Inline mempcpy

Hi,
As pointed out that following patch should be generic
http://patchwork.sourceware.org/patch/6459/
here is sample patch that does it. Header for mempcpy now becomes
following:

#ifdef __USE_GNU
# if !defined _HAVE_STRING_ARCH_mempcpy || defined _FORCE_INLINES

#define __mempcpy(dest, src, n) __mempcpy_inline(dest, src, n)

__STRING_INLINE void *
__mempcpy_inline (void *__restrict __dest,
                  const void *__restrict __src, size_t __n)
{
  return memcpy (__dest, __src, __n) + __n;
}

# endif
#endif

Patch itself is messy as it also removes obsolete inlining for gcc-3.4
and older. Ok to clean that up or should I send separate patch to remove
all obsolete inlines from string2.h. These would also cause regression
as implementations improved a lot and inlines there use only 32bit
access without using 64bit capabilities.

Comments?

	* string/bits/string2.h: Use inline mempcpy.

On Wed, 13 May 2015, Ondřej Bílka wrote:

> Hi,
> As pointed out that following patch should be generic
> http://patchwork.sourceware.org/patch/6459/
> here is sample patch that does it. Header for mempcpy now becomes
> following:
> 
> #ifdef __USE_GNU
> # if !defined _HAVE_STRING_ARCH_mempcpy || defined _FORCE_INLINES

Doesn't this change the semantics of _HAVE_STRING_ARCH_mempcpy?  That is, 
after this patch, architectures with efficient .S implementations of 
mempcpy (as opposed to ones that just use the default .c implementation) 
should define that macro rather than just those with inline 
implementations.  So the patch needs to update architectures as well.

> Patch itself is messy as it also removes obsolete inlining for gcc-3.4
> and older. Ok to clean that up or should I send separate patch to remove
> all obsolete inlines from string2.h. These would also cause regression
> as implementations improved a lot and inlines there use only 32bit
> access without using 64bit capabilities.

Did the discussion involving 
<https://sourceware.org/ml/libc-alpha/2013-01/msg00157.html> and 
<https://sourceware.org/ml/libc-alpha/2013-01/msg00270.html> reach any 
wiki-documented consensus regarding what compiler versions it's worth 
having any optimizations for in the headers?

On Wed, May 13, 2015 at 10:21:25PM +0000, Joseph Myers wrote:
> On Wed, 13 May 2015, Ondřej Bílka wrote:
> 
> > Hi,
> > As pointed out that following patch should be generic
> > http://patchwork.sourceware.org/patch/6459/
> > here is sample patch that does it. Header for mempcpy now becomes
> > following:
> > 
> > #ifdef __USE_GNU
> > # if !defined _HAVE_STRING_ARCH_mempcpy || defined _FORCE_INLINES
> 
> Doesn't this change the semantics of _HAVE_STRING_ARCH_mempcpy?  That is, 
> after this patch, architectures with efficient .S implementations of 
> mempcpy (as opposed to ones that just use the default .c implementation) 
> should define that macro rather than just those with inline 
> implementations.  So the patch needs to update architectures as well.
>
Read the friendly thread. In short you should always expand mempcpy to
memcpy and keep mempcpy implementation just for legacy programs.

Possible benefits are small, few cycles to save register spill at most.
It can't be better as you can implement memcpy with mempcpy.

But there is huge cost involved. Effective mempcpy is often larger than
one kilobyte. Duplicating that kilobyte for mempcpy would increase
instruction cache pressure a lot. Easily it could cause one cache miss
per call resulting additional 30 cycle penalty negating any benefit you
try to make.

On Wed, May 13, 2015 at 10:21:25PM +0000, Joseph Myers wrote:
> > Patch itself is messy as it also removes obsolete inlining for gcc-3.4
> > and older. Ok to clean that up or should I send separate patch to remove
> > all obsolete inlines from string2.h. These would also cause regression
> > as implementations improved a lot and inlines there use only 32bit
> > access without using 64bit capabilities.
> 
> Did the discussion involving 
> <https://sourceware.org/ml/libc-alpha/2013-01/msg00157.html> and 
> <https://sourceware.org/ml/libc-alpha/2013-01/msg00270.html> reach any 
> wiki-documented consensus regarding what compiler versions it's worth 
> having any optimizations for in the headers?
> 
These aren't too related to this problem, there is barely any discussion
about performance, its mostly about support.

I would use rule that if user doesn't care about performance by using
obsolete gcc that generates slower code why should we care?

Also as I said before removing many of these would improve performance
as they are obsolete and libcall is faster.

On Thu, 14 May 2015, Ondřej Bílka wrote:

> On Wed, May 13, 2015 at 10:21:25PM +0000, Joseph Myers wrote:
> > On Wed, 13 May 2015, Ondřej Bílka wrote:
> > 
> > > Hi,
> > > As pointed out that following patch should be generic
> > > http://patchwork.sourceware.org/patch/6459/
> > > here is sample patch that does it. Header for mempcpy now becomes
> > > following:
> > > 
> > > #ifdef __USE_GNU
> > > # if !defined _HAVE_STRING_ARCH_mempcpy || defined _FORCE_INLINES
> > 
> > Doesn't this change the semantics of _HAVE_STRING_ARCH_mempcpy?  That is, 
> > after this patch, architectures with efficient .S implementations of 
> > mempcpy (as opposed to ones that just use the default .c implementation) 
> > should define that macro rather than just those with inline 
> > implementations.  So the patch needs to update architectures as well.
> >
> Read the friendly thread. In short you should always expand mempcpy to
> memcpy and keep mempcpy implementation just for legacy programs.
> 
> Possible benefits are small, few cycles to save register spill at most.
> It can't be better as you can implement memcpy with mempcpy.
> 
> But there is huge cost involved. Effective mempcpy is often larger than
> one kilobyte. Duplicating that kilobyte for mempcpy would increase
> instruction cache pressure a lot. Easily it could cause one cache miss
> per call resulting additional 30 cycle penalty negating any benefit you
> try to make.

That cost sounds like it very much depends on the architecture's cache 
size on typical processors (presumably likely to go up over time) and the 
size of the function implementations.  On SPARC, for example, mempcpy is 
simply an alternative entry point in the same file as memcpy that sets up 
a different return value, so I see no apparent reason to avoid the 
out-of-line mempcpy implementation there.  Generally, you should get 
consensus for the change from architecture people on each architecture 
with its own mempcpy version (x86_64, x86, powerpc, sparc).

Regarding the patch in particular, you appear to lose the

> -/* In glibc we use this function frequently but for namespace reasons
> -   we have to use the name `__mempcpy'.  */
> -#   define mempcpy(dest, src, n) __mempcpy (dest, src, n)

so it's not clear the patch would even be effective for programs that call 
mempcpy rather than __mempcpy.  And in

> #define __mempcpy(dest, src, n) __mempcpy_inline(dest, src, n)

you're missing the space before the '(' in the function call.  The patch 
removes the definition of __mempcpy_small from the headers, but 
__mempcpy_small is part of the libc ABI for existing binaries that were 
built with old GCC; I'd have expected this patch to cause string-inlines.c 
no longer to export a definition of __mempcpy_small, resulting in ABI 
tests failing (you didn't say how the patch was tested).  I'd expect you 
to need to arrange for the definition to go somewhere else so that it 
still gets exported from libc (taking care of x86 having its own versions 
of string-inlines.c).  And as __mempcpy_inline is __STRING_INLINE, the 
compiler could choose not to inline it, meaning that it needs to go in the 
Versions file and ABI baselines unless you can arrange for it to have 
__mempcpy as its asm name for any non-inlined calls (which would certainly 
be preferable, to avoid introducing a new ABI).

All those ABI complications around removing existing inlines from headers 
definitely show that any such removals should be separate from adding new 
inlines or other optimizations.

On Thu, 14 May 2015, Ondřej Bílka wrote:

> > Did the discussion involving 
> > <https://sourceware.org/ml/libc-alpha/2013-01/msg00157.html> and 
> > <https://sourceware.org/ml/libc-alpha/2013-01/msg00270.html> reach any 
> > wiki-documented consensus regarding what compiler versions it's worth 
> > having any optimizations for in the headers?
> > 
> These aren't too related to this problem, there is barely any discussion
> about performance, its mostly about support.

It seems directly related.  
<https://sourceware.org/ml/libc-alpha/2013-01/msg00270.html> has two 
paragraphs about optimizations in headers and not regressing such support 
"just in the name of generic simplification".

> I would use rule that if user doesn't care about performance by using
> obsolete gcc that generates slower code why should we care?

Well, maybe we should say that the optimizations for GCC < 4.1 (say) 
aren't significant enough to be worth the complexity of keeping (in 
headers not shared with gnulib) - though you still need to export all the 
old ABIs exported by string-inlines.c (they could be conditioned 
SHLIB_COMPAT so new architectures don't get them, however).  But such a 
change needs consensus.

> Also as I said before removing many of these would improve performance
> as they are obsolete and libcall is faster.

These are generally about optimizing cases of e.g. constant arguments for 
old compilers that didn't have such optimizations themselves.

On 05/14/2015 09:47 AM, Joseph Myers wrote:
> Well, maybe we should say that the optimizations for GCC < 4.1 (say)
> aren't significant enough to be worth the complexity of keeping (in
> headers not shared with gnulib)

We don't care about optimizing older GCC for Gnulib either, so please 
don't bother to keep that complexity just for Gnulib's sake.

On 14-05-2015 13:40, Joseph Myers wrote:
> On Thu, 14 May 2015, Ondřej Bílka wrote:
> 
>> On Wed, May 13, 2015 at 10:21:25PM +0000, Joseph Myers wrote:
>>> On Wed, 13 May 2015, Ondřej Bílka wrote:
>>>
>>>> Hi,
>>>> As pointed out that following patch should be generic
>>>> http://patchwork.sourceware.org/patch/6459/
>>>> here is sample patch that does it. Header for mempcpy now becomes
>>>> following:
>>>>
>>>> #ifdef __USE_GNU
>>>> # if !defined _HAVE_STRING_ARCH_mempcpy || defined _FORCE_INLINES
>>>
>>> Doesn't this change the semantics of _HAVE_STRING_ARCH_mempcpy?  That is, 
>>> after this patch, architectures with efficient .S implementations of 
>>> mempcpy (as opposed to ones that just use the default .c implementation) 
>>> should define that macro rather than just those with inline 
>>> implementations.  So the patch needs to update architectures as well.
>>>
>> Read the friendly thread. In short you should always expand mempcpy to
>> memcpy and keep mempcpy implementation just for legacy programs.
>>
>> Possible benefits are small, few cycles to save register spill at most.
>> It can't be better as you can implement memcpy with mempcpy.
>>
>> But there is huge cost involved. Effective mempcpy is often larger than
>> one kilobyte. Duplicating that kilobyte for mempcpy would increase
>> instruction cache pressure a lot. Easily it could cause one cache miss
>> per call resulting additional 30 cycle penalty negating any benefit you
>> try to make.
> 
> That cost sounds like it very much depends on the architecture's cache 
> size on typical processors (presumably likely to go up over time) and the 
> size of the function implementations.  On SPARC, for example, mempcpy is 
> simply an alternative entry point in the same file as memcpy that sets up 
> a different return value, so I see no apparent reason to avoid the 
> out-of-line mempcpy implementation there.  Generally, you should get 
> consensus for the change from architecture people on each architecture 
> with its own mempcpy version (x86_64, x86, powerpc, sparc).

Back when I added mempcpy for POWER7, I just used the memcpy as base and
optimize it to avoid the stack call allocation.  However, I tend to agree
that for this particular function icache pressure is more dominant that
a function call. So I think for powerpc the inline is better.

> 
> Regarding the patch in particular, you appear to lose the
> 
>> -/* In glibc we use this function frequently but for namespace reasons
>> -   we have to use the name `__mempcpy'.  */
>> -#   define mempcpy(dest, src, n) __mempcpy (dest, src, n)
> 
> so it's not clear the patch would even be effective for programs that call 
> mempcpy rather than __mempcpy.  And in
> 
>> #define __mempcpy(dest, src, n) __mempcpy_inline(dest, src, n)
> 
> you're missing the space before the '(' in the function call.  The patch 
> removes the definition of __mempcpy_small from the headers, but 
> __mempcpy_small is part of the libc ABI for existing binaries that were 
> built with old GCC; I'd have expected this patch to cause string-inlines.c 
> no longer to export a definition of __mempcpy_small, resulting in ABI 
> tests failing (you didn't say how the patch was tested).  I'd expect you 
> to need to arrange for the definition to go somewhere else so that it 
> still gets exported from libc (taking care of x86 having its own versions 
> of string-inlines.c).  And as __mempcpy_inline is __STRING_INLINE, the 
> compiler could choose not to inline it, meaning that it needs to go in the 
> Versions file and ABI baselines unless you can arrange for it to have 
> __mempcpy as its asm name for any non-inlined calls (which would certainly 
> be preferable, to avoid introducing a new ABI).
> 
> All those ABI complications around removing existing inlines from headers 
> definitely show that any such removals should be separate from adding new 
> inlines or other optimizations.
>