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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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?


> 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).

> 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.

> 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.

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

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

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com