[PATCH] powerpc: Optimize memrchr for power8

[Rajalakshmi Srinivasaraghavan]

On 09/29/2017 08:26 PM, Gabriel F. T. Gomes wrote:
> On 19 Sep 2017, Rajalakshmi Srinivasaraghavan wrote:
> 
>> --- a/sysdeps/powerpc/powerpc64/multiarch/memrchr-ppc64.c
>> +++ b/sysdeps/powerpc/powerpc64/multiarch/memrchr-ppc64.c
>> @@ -16,4 +16,16 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>
>> -#include <sysdeps/powerpc/powerpc32/power4/multiarch/memrchr-ppc32.c>
>> +#include <string.h>
>> +
>> +#define MEMRCHR  __memrchr_ppc
>> +
>> +#undef weak_alias
>> +#define weak_alias(a, b)
>> +
>> +# undef libc_hidden_builtin_def
>> +# define libc_hidden_builtin_def(name)
>     ^
> This space is not needed.

Ack.

> 
>> --- /dev/null
>> +++ b/sysdeps/powerpc/powerpc64/power8/memrchr.S
>> @@ -0,0 +1,321 @@
>> +/* Optimized memrchr implementation for PowerPC64/POWER8.
>> +   Copyright (C) 2017 Free Software Foundation, Inc.
>> +   Contributed by Luis Machado <luisgpm@br.ibm.com>.
> 
> No "Contributed by" statements in new files.
> 

I dont think its needed now as mentioned here.
https://sourceware.org/glibc/wiki/Contribution%20checklist

>> +/* TODO: change these to the actual instructions when the minimum required
>> +   binutils allows it.  */
>> +#define MTVRD(v, r) .long (0x7c000167 | ((v)<<(32-11)) | ((r)<<(32-16)))
>> +#define MFVRD(r, v) .long (0x7c000067 | ((v)<<(32-11)) | ((r)<<(32-16)))
> 
> OK
> 
> 
> I focused the review on the new code blocks (as compared to the power7
> implementation).  They look good, I only have minor comments.

Thanks for the review.

> 
>> +	.align	4
>> +	/* At this point, r8 is 16B aligned.  */
>> +L(align_qw):
>> [...]
> 
> OK.
> 
>> +	/* Handle r5 > 64.  Loop over the bytes in strides of 64B.  */
>> +	.align 4
>> +L(loop):
>> [...]
> 
> OK.
> 
>> +	/* Handle remainder of 64B loop or r5 > 64.  */
>> +	.align	4
>> +L(tail64):
>> [...]
> 
> OK.
> 
>> +	/* Found a match in 64B loop.  */
>> +	.align	4
>> +L(found):
>> +	/* Permute the first bit of each byte into bits 48-63.  */
>> +	VBPERMQ(v6, v6, v10)
>> +	VBPERMQ(v7, v7, v10)
>> +	VBPERMQ(v8, v8, v10)
>> +	VBPERMQ(v9, v9, v10)
>> +	/* Shift each component into its correct position for merging.  */
>> +#ifdef __LITTLE_ENDIAN__
>> +	vsldoi	v7, v7, v7, 2
>> +	vsldoi	v8, v8, v8, 4
>> +	vsldoi	v9, v9, v9, 6
>> +#else
>> +	vsldoi	v6, v6, v6, 6
>> +	vsldoi	v7, v7, v7, 4
>> +	vsldoi	v8, v8, v8, 2
>> +#endif
>> +	/* Merge the results and move to a GPR.  */
>> +	vor	v11, v6, v7
>> +	vor	v4, v9, v8
>> +	vor	v4, v11, v4
>> +	MFVRD(r5, v4)
>> +#ifdef __LITTLE_ENDIAN__
>> +	cntlzd	r6, r5	/* Count leading zeros before the match.  */
>> +#else
>> +	addi	r6, r5, -1
>> +	andc	r6, r6, r5
>> +	popcntd	r6, r6
>> +#endif
>> +	addi	r8, r8, 63
>> +	sub	r3, r8, r6	/* Compute final length.  */
>                                     ~~~~~~~~~~~~~~~~~~~~
> This comment is a bit misleading.
> Maybe replace 'length' with 'address' or 'pointer'?

Ack.

> 
>> +	blr
>> +
>> +	/* Found a match in last 16 bytes.  */
>> +	.align	4
>> +L(found_16B):
>> +	/* Permute the first bit of each byte into bits 48-63.  */
>> +	VBPERMQ(v6, v6, v10)
>> +	/* Shift each component into its correct position for merging.  */
>> +#ifdef __LITTLE_ENDIAN__
>> +	vsldoi	v6, v6, v6, 6
>> +	MFVRD(r7, v6)
>> +	cntlzd	r6, r7	/* Count leading zeros before the match.  */
>> +#else
>> +	MFVRD(r7, v6)
>> +	addi	r6, r7, -1
>> +	andc	r6, r6, r7
>> +	popcntd	r6, r6
>> +#endif
>> +	addi	r8, r8, 15
>> +	sub	r3, r8, r6	/* Compute final length.  */
>                                     ~~~~~~~~~~~~~~~~~~~~
> Likewise.
> 
> 
> Your patch looks good to me with these small changes.

Pushed as 59ba2d2b5421.

> 
> Reviewed-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br>
> 
> 
> PS: It's the first time I use the reviewed-by tag in this mailing list.
> This and further uses imply what was discussed in libc-alpha [1],
> particularly, it implies that I understand and agree with the "Reviewer's
> statement of oversight" [2] used in the linux kernel community.  Feel free
> to add this Reviewed-by statement to your commit message if you want to.
> 
> [1] https://sourceware.org/ml/libc-alpha/2017-09/msg00816.html
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
> 
> 

-- 
Thanks
Rajalakshmi S