Re: [PATCH] powerpc64le: Optimize memset for POWER10

[Raoni Fassina Firmino <raoni@linux.ibm.com>]

Thanks for the review Matheus, let me know if I missed something.

> > +L(_memset):
> > +	/* Assume memset of zero length is uncommon, and just let it go
> > +	   through the small path below.  */
> > +	cmpldi	r5,64
> > +
> > +	/* Replicate byte to quad word.  */
> > +	mtvsrws v0+32,r4
> > +	vspltb	v0,v0,15
> 
> Why not simply use mtvsrd here? The byte splat part will have to be done
> separately anyways:
> 
>       mtvsrd v0+32,r4
>       vspltb v0,v0,7

Done.

Maybe this one is more intuitive. Don't have to wonder if the previous
word splat.


> > +	sub.	r11,r5,r8
> > +	isellt	r11,0,r11	/* Saturate the subtraction to zero.  */
> > +
> > +	stxvl	v0+32,r3,r5
> > +	stxvl	v0+32,r10,r11
> > +
> > +	addi	r9,r3,32
> > +	addi	r10,r3,48
> > +
> > +	sub.	r11,r11,r8
> > +	isellt	r11,0,r11
> > +
> > +	sub.	r5,r11,r8
> > +	isellt	r5,0,r5
> 
> Minor detail: I see this construct appears many times in the code. You
> could create a macro for it like:
> 
> #define SUBS(rt,ra,rb)   \
>         sub.   rt,ra,rb; \
>         isellt rt,0,rt;

I had this before, but I got to the conclusion it was hindering seen all
the code. Since it is only two instructions I though the macro benefits
were marginal and added some noise to have macro and mnemonics mixed up
in the code. There was a practical reason though, in one case I have a
blelr in the tail, so the macro could not be use in this case anyway.

But is there is a strong opinion here I can change it.


> > +
> > +	stxvl	v0+32,r9,r11
> > +	stxvl	v0+32,r10,r5
> > +
> > +	blr
> 
> Ok. Takes advantage of the fact that if the length passed to stxvl is
> zero, nothing is done.
> 
> Maybe you could expand the comment at the beginning of this block to
> make that trick clear.

Done.

 
> > +
> > +	.balign	16
> > +L(large):
> > +	mr	r6,r3	/* Don't modify r3 since we need to return it.  */
> > +
> > +	/* Get dest 16B aligned.  */
> > +	neg	r0,r3
> > +	clrldi.	r7,r0,(64-4)
> > +	beq	L(aligned)
> > +	rldic	r9,r0,56,4	/* (~X & 0xf)<<56 "clrlsldi r9,r0,64-4,56".  */
> 
> Why not just use clrlsldi as noted in the comment? It makes clearer what
> the instruction is doing.

Because of this:

	clrldi.	r7,r0,(64-4)
	beq	L(aligned)
	clrlsldi	r9,r0,64-4,56

It is too big to stay in the same tab column ad the rest of the
instructions and the options to fix this weren't great. It was just a
formatting decision, hence the comment to try to help out a little.
Also, in this particular case rldic reading of the values is not that
terrible, it is like: Keep the 4 least significant bits, shift then 56
bits left.

I can change if you or anyone else feel strong about it.


> > +
> > +	/* After alignment, if there is 127B or less left
> > +	   go directly to the tail.  */
> 
> 127B -> 63B

Done.


> > +	cmpldi	r5,64
> > +	blt	L(tail_64)
> > +
> > +	.balign	16
> > +L(aligned):
> > +	srdi.	r0,r5,7
> > +	beq	L(tail_128)
> > +
> > +	cmpldi	cr5,r5,255
> > +	cmpldi	cr6,r4,0
> > +	crand	27,26,21
> > +	bt	27,L(dcbz)
> 
> This last block deserves a comment, as it is really hard to follow.
> 
> IIUC, this is what this code is checking:
> 
>      if r5 > 255 && r4 == 0
>         goto L(dcbz)
> 
> So if we have at least 256B left and we are setting zeroes, then use the
> dcbz strategy. Ok.

Done.

I did the comment in one line, but mostly the same, let me know if it
explain it well. I decided to use the arguments names instead of
registers in hope to be more meaningful, but lets see it is not more
confusing.

> > +	.balign	16
> > +L(dcbz):
> > +	/* Special case when value is 0 and we have a long length to deal
> > +	   with.  Use dcbz to zero out a full cacheline of 128 bytes at a time.
> > +	   Before using dcbz though, we need to get the destination 128-byte
> > +	   aligned.  */
> > +	neg	r0,r6
> > +	clrldi.	r0,r0,(64-7)
> > +	beq	L(dcbz_aligned)
> > +
> > +	sub	r5,r5,r0
> > +	mtocrf	0x2,r0	/* These are the bits 57..59, the ones for sizes 64,
> > +			   32 and 16 which are those that need to be check.  */
> 
> need to be check -> need to be checked
> 
> Please add to the comment that these bits are being set to cr6. mtocrf
> is not one of the most straightfoward instructions to read =/

Done.

> > +	/* Write 16~128 bytes until DST is aligned to 128 bytes.  */
> 
> 16~128 -> 16-128

Done.


> > +	.balign	16
> > +L(bcdz_tail):
> > +	/* We have 1~511 bytes remaining.  */
> 
> 1~511 -> 1-511

Done.


> > +END_GEN_TB (MEMSET,TB_TOCLESS)
> > +libc_hidden_builtin_def (memset)
> > +
> > +/* Copied from bzero.S to prevent the linker from inserting a stub
> > +   between bzero and memset.  */
> > +ENTRY_TOCLESS (__bzero)
> > +	CALL_MCOUNT 3
> 
> Should this CALL_MCOUNT 2 since bzero receives just 2 args?

Done.

You are right here, good catch.


o/
Raoni