[PATCH 3/3] sparc: M7 optimized memcpy/mempcpy/memmove/memset/bzero.
Patrick McGehearty
patrick.mcgehearty@oracle.com
Thu Sep 28 19:44:00 GMT 2017
Responses after the comments. Apologies for the earlier incomplete message.
On 9/28/2017 11:17 AM, Adhemerval Zanella wrote:
>
> On 27/09/2017 13:09, Patrick McGehearty wrote:
>> Tested in sparcv9-*-* and sparc64-*-* targets in both multi and
>> non-multi arch configurations.
>>
>> Support added to identify Sparc M7/T7/S7/M8/T8 processor capability.
>> Usual "make check" correctness tests run with no regressions.
>> Performance tests run on Sparc S7 using new code and old niagara4 code.
>>
>> Optimizations for memcpy also apply to mempcpy and memmove
>> where they share code. Optimizations for memset also apply
>> to bzero as they share code.
>>
>> For memcpy/mempcpy/memmove, performance comparison with niagara4 code:
>> Long word aligned data
>> 0-127 bytes - minimal changes
>> 128-1023 bytes - 7-30% gain
>> 1024+ bytes - 1-7% gain (in cache); 30-100% gain (out of cache)
>> Word aligned data
>> 0-127 bytes - 50%+ gain
>> 128-1023 bytes - 10-200% gain
>> 1024+ bytes - 0-15% gain (in cache); 5-50% gain (out of cache)
>> Unaligned data
>> 0-127 bytes - 0-70%+ gain
>> 128-447 bytes - 40-80%+ gain
>> 448-511 bytes - 1-3% loss
>> 512-4096 bytes - 2-3% gain (in cache); 0-20% gain (out of cache)
>> 4096+ bytes - +/- 3% (in cache); 20-50% gain (out of cache)
>>
>> For memset/bzero, performance comparison with niagara4 code:
>> For memset nonzero data,
>> 256-1023 bytes - 60-90% gain (in cache); 5% gain (out of cache)
>> 1K+ bytes - 80-260% gain (in cache); 40-80% gain (out of cache)
>> For memset zero data (and bzero),
>> 256-1023 bytes - 80-120% gain (in cache), 0% gain (out of cache)
>> 1024+ bytes - 2-4x gain (in cache), 10-35% gain (out of cache)
>> ---
>> ChangeLog | 20 +
>> sysdeps/sparc/sparc32/sparcv9/multiarch/Makefile | 3 +-
>> .../sparcv9/multiarch/memcpy-memmove-niagara7.S | 2 +
>> sysdeps/sparc/sparc32/sparcv9/multiarch/memmove.S | 2 +
>> .../sparc32/sparcv9/multiarch/memset-niagara7.S | 2 +
>> .../sparc/sparc32/sparcv9/multiarch/rtld-memmove.c | 1 +
>> sysdeps/sparc/sparc64/multiarch/Makefile | 3 +-
>> sysdeps/sparc/sparc64/multiarch/ifunc-impl-list.c | 13 +
>> .../sparc64/multiarch/memcpy-memmove-niagara7.S | 979 ++++++++++++++++++++
>> sysdeps/sparc/sparc64/multiarch/memcpy.S | 28 +-
>> sysdeps/sparc/sparc64/multiarch/memmove.S | 72 ++
>> sysdeps/sparc/sparc64/multiarch/memset-niagara7.S | 334 +++++++
>> sysdeps/sparc/sparc64/multiarch/memset.S | 28 +-
>> sysdeps/sparc/sparc64/multiarch/rtld-memmove.c | 1 +
>> 14 files changed, 1482 insertions(+), 6 deletions(-)
>> create mode 100644 sysdeps/sparc/sparc32/sparcv9/multiarch/memcpy-memmove-niagara7.S
>> create mode 100644 sysdeps/sparc/sparc32/sparcv9/multiarch/memmove.S
>> create mode 100644 sysdeps/sparc/sparc32/sparcv9/multiarch/memset-niagara7.S
>> create mode 100644 sysdeps/sparc/sparc32/sparcv9/multiarch/rtld-memmove.c
>> create mode 100644 sysdeps/sparc/sparc64/multiarch/memcpy-memmove-niagara7.S
>> create mode 100644 sysdeps/sparc/sparc64/multiarch/memmove.S
>> create mode 100644 sysdeps/sparc/sparc64/multiarch/memset-niagara7.S
>> create mode 100644 sysdeps/sparc/sparc64/multiarch/rtld-memmove.c
> I would prefer if you split this patch in a memcpy/mempcpy/memmove and
> a memset/bzero one since they are separated implementations.
I can see your point, and if I had started that way, it would not have
been a problem. At this point,
it would take some effort to redo the patch structure at this point
given our internal development
process and might significantly delay the completion of this patch due
to requirements to make
progress on other tasks. From a user's point of view, the optimizations
for memcpy on M7/T7/S7
and memset on M7/T7/S7 both are driven by the use of block initializing
store as a 64 byte operation
instead of a 32 byte operation. (side note: block initializing store on
M8/T8 is also 64 bytes, so these
optimizations are also appropriate for the new M8/T8 systems). Thus, a
user should be equally
willing to use both or neither of the new memcpy/memset optimizations.
>
> I also noticed this whitespace issues, so you might want to check this out:
>
> ---
>
> patch03:886: trailing whitespace.
> * lines from memory. Use ST_CHUNK stores to first element of each cache
> patch03:890: space before tab in indent.
> andn %o2, 0x3f, %o5 /* %o5 is multiple of block size */
> patch03:891: space before tab in indent.
> and %o2, 0x3f, %o2 /* residue bytes in %o2 */
> patch03:892: trailing whitespace.
>
> patch03:1216: new blank line at EOF.
> +
> warning: 5 lines add whitespace errors.
> ---
I've cleaned up the whitespace issues. Thank you for catching them.
>
>> diff --git a/sysdeps/sparc/sparc64/multiarch/memcpy.S b/sysdeps/sparc/sparc64/multiarch/memcpy.S
>> index b6396ee..d72f4b1 100644
>> --- a/sysdeps/sparc/sparc64/multiarch/memcpy.S
>> +++ b/sysdeps/sparc/sparc64/multiarch/memcpy.S
>> @@ -27,7 +27,19 @@ ENTRY(memcpy)
>> # ifdef SHARED
>> SETUP_PIC_REG_LEAF(o3, o5)
>> # endif
>> - set HWCAP_SPARC_CRYPTO, %o1
>> + set HWCAP_SPARC_ADP, %o1
>> + andcc %o0, %o1, %g0
>> + be 1f
>> + nop
>> +# ifdef SHARED
>> + sethi %gdop_hix22(__memcpy_niagara7), %o1
>> + xor %o1, %gdop_lox10(__memcpy_niagara7), %o1
>> +# else
>> + set __memcpy_niagara7, %o1
>> +# endif
>> + ba 10f
>> + nop
>> +1: set HWCAP_SPARC_CRYPTO, %o1
>> andcc %o0, %o1, %g0
>> be 1f
>> andcc %o0, HWCAP_SPARC_N2, %g0
>> @@ -89,7 +101,19 @@ ENTRY(__mempcpy)
>> # ifdef SHARED
>> SETUP_PIC_REG_LEAF(o3, o5)
>> # endif
>> - set HWCAP_SPARC_CRYPTO, %o1
>> + set HWCAP_SPARC_ADP, %o1
>> + andcc %o0, %o1, %g0
>> + be 1f
>> + nop
>> +# ifdef SHARED
>> + sethi %gdop_hix22(__mempcpy_niagara7), %o1
>> + xor %o1, %gdop_lox10(__mempcpy_niagara7), %o1
>> +# else
>> + set __mempcpy_niagara7, %o1
>> +# endif
>> + ba 10f
>> + nop
>> +1: set HWCAP_SPARC_CRYPTO, %o1
>> andcc %o0, %o1, %g0
>> be 1f
>> andcc %o0, HWCAP_SPARC_N2, %g0
> Since you are touching this file, I think it would be better to change the
> ifunc selector to a C implementation by using the __ifunc/sparc_libc_ifunc
> macros.
Again, I agree philosophically, but the actual code changes for ADP were
made many months ago.
Revising those changes now would require more work with little perceived
benefit.
>
>
>> diff --git a/sysdeps/sparc/sparc64/multiarch/memmove.S b/sysdeps/sparc/sparc64/multiarch/memmove.S
>> new file mode 100644
>> index 0000000..43bdf08
>> --- /dev/null
>> +++ b/sysdeps/sparc/sparc64/multiarch/memmove.S
>> @@ -0,0 +1,72 @@
>> +/* Multiple versions of memmove and bcopy
>> + All versions must be listed in ifunc-impl-list.c.
>> + Copyright (C) 2017 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, write to the Free
>> + Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
>> + 02111-1307 USA. */
>> +
>> +#include <sysdep.h>
>> +
>> +#if IS_IN (libc)
>> + .text
>> +ENTRY(memmove)
>> + .type memmove, @gnu_indirect_function
>> +# ifdef SHARED
>> + SETUP_PIC_REG_LEAF(o3, o5)
>> +# endif
>> + set HWCAP_SPARC_ADP, %o1
>> + andcc %o0, %o1, %g0
>> + be 1f
>> + nop
>> +# ifdef SHARED
>> + sethi %gdop_hix22(__memmove_niagara7), %o1
>> + xor %o1, %gdop_lox10(__memmove_niagara7), %o1
>> +# else
>> + set __memmove_niagara7, %o1
>> +# endif
>> + ba 10f
>> + nop
>> +1:
>> +# ifdef SHARED
>> + sethi %gdop_hix22(__memmove_ultra1), %o1
>> + xor %o1, %gdop_lox10(__memmove_ultra1), %o1
>> +# else
>> + set __memmove_ultra1, %o1
>> +# endif
>> +10:
>> +# ifdef SHARED
>> + add %o3, %o1, %o1
>> +# endif
>> + retl
>> + mov %o1, %o0
>> +END(memmove)
>> +
>> +libc_hidden_builtin_def (memmove)
>> +
>> +#undef libc_hidden_builtin_def
>> +#define libc_hidden_builtin_def(name)
>> +#undef weak_alias
>> +#define weak_alias(x, y)
>> +#undef libc_hidden_def
>> +#define libc_hidden_def(name)
>> +
>> +#define memmove __memmove_ultra1
>> +#define __memmove __memmove_ultra1
>> +
>> +#endif
>> +
>> +#include "../memmove.S"
>> +
> As before, I think new ifunc resolvers should be avoid to be coded in assembly.
> Also there is an extra line on this file.
The extra line has been removed.
>
>> diff --git a/sysdeps/sparc/sparc64/multiarch/rtld-memmove.c b/sysdeps/sparc/sparc64/multiarch/rtld-memmove.c
>> new file mode 100644
>> index 0000000..66fe118
>> --- /dev/null
>> +++ b/sysdeps/sparc/sparc64/multiarch/rtld-memmove.c
>> @@ -0,0 +1 @@
>> +#include <../rtld-memmove.c>
> Avoid using relative patch, include the default memmove using full paths.
>
>
I'll fix the relative paths.
- patrick
More information about the Libc-alpha
mailing list