Re: [PATCH] Inline mempcpy

[Joseph Myers <joseph at codesourcery dot com>]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com