Re: [PATCH] PowerPC: optimized memmove for POWER7/PPC64

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

Thanks for review Ulrich.


On 07-07-2014 11:51, Ulrich Weigand wrote:
> Adhemerval Zanella wrote:
>
>> 	* string/bcopy.c: Use full path to include memmove.c.
>> 	* sysdeps/powerpc/powerpc64/multiarch/Makefile: Add memmove and bcopy
>> 	multiarch objects.
>> 	* sysdeps/powerpc/powerpc64/multiarch/bcopy-ppc64.c: New file: default
>> 	bcopy for powerpc64.
>> 	* sysdeps/powerpc/powerpc64/multiarch/bcopy.c: New file: multiarch
>> 	bcopy for powerpc64.
>> 	* sysdeps/powerpc/powerpc64/multiarch/ifunc-impl-list.c: Add bcopy
>> 	and memmove implementations.
>> 	* sysdeps/powerpc/powerpc64/multiarch/memmove-power7.S: New file:
>> 	optimized multiarch memmove for POWER7/powerpc64.
>> 	* sysdeps/powerpc/powerpc64/multiarch/memmove-ppc64.c: New file:
>> 	default multiarch memmove for powerpc64.
>> 	* sysdeps/powerpc/powerpc64/multiarch/memmove.c: New file: memmove
>> 	multiarch for powerpc64.
>> 	* sysdeps/powerpc/powerpc64/power7/bcopy.c: New file: optimized bcopy
>> 	for POWER7/powerpc64.
>> 	* sysdeps/powerpc/powerpc64/power7/memmove.S: New file: optimized
>> 	memmove for POWER7/powerpc64.
> Looks good to me.  A couple of things I was wondering about:
>
>> diff --git a/string/bcopy.c b/string/bcopy.c
>> index 7c1225c..f497b5d 100644
>> --- a/string/bcopy.c
>> +++ b/string/bcopy.c
>> @@ -25,4 +25,4 @@
>>  #define	a2		dest
>>  #define	a2const
>>  
>> -#include <memmove.c>
>> +#include <string/memmove.c>
> Not sure why this is needed -- could this have an impact on any other platform?

Without full path, sysdeps/powerpc/powerpc64/multiarch/bcopy-ppc64.c will try to

#include <string/bcopy.c>

And 'string/bcopy.c' will select 'sysdeps/powerpc/powerpc64/multiarch/memmove.c'
instead of 'string/memmove.c'.  Currently no other chars reimplements bcopy.c 
(and thus including string/memmove.c will hold correct expectation) and other
archs that implements bcopy to use IFUNC (i386, x86_64) uses specialized
assembly versions.

>
>
>> diff --git a/sysdeps/powerpc/powerpc64/multiarch/memmove-power7.S b/sysdeps/powerpc/powerpc64/multiarch/memmove-power7.S
>> +#undef EALIGN
>> +#define EALIGN(name, alignt, words)				\
>> +  .section ".text";						\
>> +  ENTRY_2(__memmove_power7)					\
>> +  .align ALIGNARG(alignt);					\
>> +  EALIGN_W_##words;						\
>> +  BODY_LABEL(__memmove_power7):					\
>> +  cfi_startproc;						\
>> +  LOCALENTRY(__memmove_power7)
>> +
>> +#undef END_GEN_TB
>> +#define END_GEN_TB(name, mask)					\
>> +  cfi_endproc;							\
>> +  TRACEBACK_MASK(__memmove_power7,mask)				\
>> +  END_2(__memmove_power7)
>> +
>> +#undef libc_hidden_builtin_def
>> +#define libc_hidden_builtin_def(name)
> These strike me as odd -- but then again I see other files in multiarch/
> have similar code ...  Longer-term I guess it might be good to avoid
> all that duplication (in case someone wants to modify the common-code
> EALIGN definition).

Indeed and the other archs (more specifically i386/x86_64) I used as base do the
same.  I will check if a cleanup in this case is possible (in future patches) is
possible.

>
>> diff --git a/sysdeps/powerpc/powerpc64/multiarch/memmove.c b/sysdeps/powerpc/powerpc64/multiarch/memmove.c
>> +#if defined SHARED && !defined NOT_IN_libc
> [snip]
>> +#else
>> +# include <string/memmove.c>
>> +#endif
> Some files have that #include of the baseline file, others don't --
> not sure I understand exactly why/when this is needed.

Basically this is the case of loader build (rtld-memmove.c) and for some symbols
(memcmp, memcpy, etc.) using IFUNC is not an option (since such algorithms are
required before ifunc relocation mechanism is started).  For these cases it just
build the default memmove implementation. Another way, which is the case for
some implementation, is create a rtld-<algorithm>.c.

>
>> diff --git a/sysdeps/powerpc/powerpc64/power7/memmove.S b/sysdeps/powerpc/powerpc64/power7/memmove.S
>> +/* void* [r3] memcpy (void *dest [r3], const void *src [r4], size_t len [r5])
> Should be memmove.

Thanks, fixed.

>
> The assembler implementation looks good as far as I can see; it certainly
> would be good to have testcases for all the various cases ...

GLIBC testcases in fact stressed all the backwards paths (the forward case is just a memcpy
case, which is already tested).

>
> Also, this represents a significant duplication of code with the memcpy
> implementation; is there no way to have just a single copy?

Yeah, that is matter of opinion: some powerpc optimizations I have pushed some time ago
generate the same concern and my idea is I prefer this code duplication than the 
add 2, 3, or more similar string optimization in the same file (as for strcmp.S for
x86_64 for instance).

But I'll check later if how complicate a memcpy refactor to hold memmove logic would be.

>
> Bye,
> Ulrich
>