[MIPS] Modify memcpy.S for mips32r6/mips64r6

  On Mon, 2014-12-22 at 17:59 +0000, Joseph Myers wrote:
> On Fri, 19 Dec 2014, Steve Ellcey  wrote:
> 
> > 	* sysdeps/mips/memcpy.S: Fix preprocessor indentation.
> 
> Please separate the formatting fixes from the substantive changes.  The 
> formatting fixes - a patch that shows no changes from "git diff -w" - can 
> go in as obvious.  The r6 changes should then be resubmitted.

Here is a new memcpy patch.  It has just the changes needed for
mips32r6/mips64r6 support.  Note that there are still some preprocessor
indentation changes where existing ifdefs are now under a newly
introduced !R6_CODE ifdef.

Tested with the mips32r6/mips64r6 GCC, binutils and qemu simulator.

OK to checkin?

Steve Ellcey
sellcey@imgtec.com


2014-12-22  Steve Ellcey  <sellcey@imgtec.com>

	* sysdeps/mips/memcpy.S: Add support for mips32r6/mips64r6.
  

On Tue, Dec 23, 2014 at 09:08:46AM -0800, Steve Ellcey wrote:
> On Mon, 2014-12-22 at 17:59 +0000, Joseph Myers wrote:
> > On Fri, 19 Dec 2014, Steve Ellcey  wrote:
> > 
> > > 	* sysdeps/mips/memcpy.S: Fix preprocessor indentation.
> > 
> > Please separate the formatting fixes from the substantive changes.  The 
> > formatting fixes - a patch that shows no changes from "git diff -w" - can 
> > go in as obvious.  The r6 changes should then be resubmitted.
> 
> Here is a new memcpy patch.  It has just the changes needed for
> mips32r6/mips64r6 support.  Note that there are still some preprocessor
> indentation changes where existing ifdefs are now under a newly
> introduced !R6_CODE ifdef.
> 
> Tested with the mips32r6/mips64r6 GCC, binutils and qemu simulator.
> 
> OK to checkin?
> 
still contains likely performance regression. using indirect jumps is
slow, try different approaches.
  

On Tue, 2014-12-23 at 18:25 +0100, Ondřej Bílka wrote:

> > Here is a new memcpy patch.  It has just the changes needed for
> > mips32r6/mips64r6 support.  Note that there are still some preprocessor
> > indentation changes where existing ifdefs are now under a newly
> > introduced !R6_CODE ifdef.
> > 
> > Tested with the mips32r6/mips64r6 GCC, binutils and qemu simulator.
> > 
> > OK to checkin?
> > 
> still contains likely performance regression. using indirect jumps is
> slow, try different approaches.

There is no performance regression for existing MIPS architectures
because that code has not changed.  The code may not be optimal for
mips32r6/mips64r6 (yet) but I would rather get the functionality in
now and optimize it later instead of trying to get it perfect
immediately.  Especially with a release coming up soon.

Steve Ellcey
sellcey@imgtec.com
  

On 12/23/2014 09:08 AM, Steve Ellcey wrote:
> +	andi	t8,a0,7
> +	lapc	t9,L(atable)
> +	PTR_LSA	t9,t8,t9,2
> +	jrc	t9
> +L(atable):
> +	bc	L(lb0)
> +	bc	L(lb7)
> +	bc	L(lb6)
> +	bc	L(lb5)
> +	bc	L(lb4)
> +	bc	L(lb3)
> +	bc	L(lb2)
> +	bc	L(lb1)
> +L(lb7):
> +	lb	a3, 6(a1)
> +	sb	a3, 6(a0)
> +L(lb6):
> +	lb	a3, 5(a1)
> +	sb	a3, 5(a0)
> +L(lb5):
> +	lb	a3, 4(a1)
> +	sb	a3, 4(a0)
> +L(lb4):
> +	lb	a3, 3(a1)
> +	sb	a3, 3(a0)
> +L(lb3):
> +	lb	a3, 2(a1)
> +	sb	a3, 2(a0)
> +L(lb2):
> +	lb	a3, 1(a1)
> +	sb	a3, 1(a0)
> +L(lb1):
> +	lb	a3, 0(a1)
> +	sb	a3, 0(a0)
L(lbx):
> +
> +	li	t9,8
> +	subu	t8,t9,t8
> +	PTR_SUBU a2,a2,t8
> +	PTR_ADDU a0,a0,t8
> +	PTR_ADDU a1,a1,t8
> +L(lb0):

This table is regular enough that I wonder if it wouldn't be better to do some
arithmetic instead of a branch-to-branch.  E.g.

	andi	t7,a0,7
	li	t8,L(lb0)-L(lbx)
	lsa	t8,t7,t8,8
	lapc	t9,L(lb0)
	selnez	t8,t8,t7
	PTR_SUBU t9,t9,t8
	jrc	t9

Which is certainly smaller than your 12 insns, unlikely to be slower on any
conceivable hardware, but probably faster on most.


r~
  

On Tue, Dec 23, 2014 at 09:52:56AM -0800, Richard Henderson wrote:
> On 12/23/2014 09:08 AM, Steve Ellcey wrote:
> > +	andi	t8,a0,7
> > +	lapc	t9,L(atable)
> > +	PTR_LSA	t9,t8,t9,2
> > +	jrc	t9
> > +L(atable):
> > +	bc	L(lb0)
> > +	bc	L(lb7)
> > +	bc	L(lb6)
> > +	bc	L(lb5)
> > +	bc	L(lb4)
> > +	bc	L(lb3)
> > +	bc	L(lb2)
> > +	bc	L(lb1)
> > +L(lb7):
> > +	lb	a3, 6(a1)
> > +	sb	a3, 6(a0)
> > +L(lb6):
> > +	lb	a3, 5(a1)
> > +	sb	a3, 5(a0)
> > +L(lb5):
> > +	lb	a3, 4(a1)
> > +	sb	a3, 4(a0)
> > +L(lb4):
> > +	lb	a3, 3(a1)
> > +	sb	a3, 3(a0)
> > +L(lb3):
> > +	lb	a3, 2(a1)
> > +	sb	a3, 2(a0)
> > +L(lb2):
> > +	lb	a3, 1(a1)
> > +	sb	a3, 1(a0)
> > +L(lb1):
> > +	lb	a3, 0(a1)
> > +	sb	a3, 0(a0)
> L(lbx):
> > +
> > +	li	t9,8
> > +	subu	t8,t9,t8
> > +	PTR_SUBU a2,a2,t8
> > +	PTR_ADDU a0,a0,t8
> > +	PTR_ADDU a1,a1,t8
> > +L(lb0):
> 
> This table is regular enough that I wonder if it wouldn't be better to do some
> arithmetic instead of a branch-to-branch.  E.g.
> 
> 	andi	t7,a0,7
> 	li	t8,L(lb0)-L(lbx)
> 	lsa	t8,t7,t8,8
> 	lapc	t9,L(lb0)
> 	selnez	t8,t8,t7
> 	PTR_SUBU t9,t9,t8
> 	jrc	t9
> 
> Which is certainly smaller than your 12 insns, unlikely to be slower on any
> conceivable hardware, but probably faster on most.
> 
Do you have that hardware? I already objected versus table but do not
have data. I wouldn't be surprised if its slower than byte-by-byte copy 
with if after each byte. Or just copy 8 bytes without condition but I am
not sure how hardware handles overlapping stores. Difference will be
bigger in practice, in profiling around 50% calls are 8 byte aligned and
you save address calculation cost on these.
  

Ondřej Bílka <neleai@seznam.cz>  writes:
> On Tue, Dec 23, 2014 at 09:52:56AM -0800, Richard Henderson wrote:
> > On 12/23/2014 09:08 AM, Steve Ellcey wrote:
> > > +	andi	t8,a0,7
> > > +	lapc	t9,L(atable)
> > > +	PTR_LSA	t9,t8,t9,2
> > > +	jrc	t9
> > > +L(atable):
> > > +	bc	L(lb0)
> > > +	bc	L(lb7)
> > > +	bc	L(lb6)
> > > +	bc	L(lb5)
> > > +	bc	L(lb4)
> > > +	bc	L(lb3)
> > > +	bc	L(lb2)
> > > +	bc	L(lb1)
> > > +L(lb7):
> > > +	lb	a3, 6(a1)
> > > +	sb	a3, 6(a0)
> > > +L(lb6):
> > > +	lb	a3, 5(a1)
> > > +	sb	a3, 5(a0)
> > > +L(lb5):
> > > +	lb	a3, 4(a1)
> > > +	sb	a3, 4(a0)
> > > +L(lb4):
> > > +	lb	a3, 3(a1)
> > > +	sb	a3, 3(a0)
> > > +L(lb3):
> > > +	lb	a3, 2(a1)
> > > +	sb	a3, 2(a0)
> > > +L(lb2):
> > > +	lb	a3, 1(a1)
> > > +	sb	a3, 1(a0)
> > > +L(lb1):
> > > +	lb	a3, 0(a1)
> > > +	sb	a3, 0(a0)
> > L(lbx):
> > > +
> > > +	li	t9,8
> > > +	subu	t8,t9,t8
> > > +	PTR_SUBU a2,a2,t8
> > > +	PTR_ADDU a0,a0,t8
> > > +	PTR_ADDU a1,a1,t8
> > > +L(lb0):
> >
> > This table is regular enough that I wonder if it wouldn't be better to
> > do some arithmetic instead of a branch-to-branch.  E.g.
> >
> > 	andi	t7,a0,7
> > 	li	t8,L(lb0)-L(lbx)
> > 	lsa	t8,t7,t8,8
> > 	lapc	t9,L(lb0)
> > 	selnez	t8,t8,t7
> > 	PTR_SUBU t9,t9,t8
> > 	jrc	t9
> >
> > Which is certainly smaller than your 12 insns, unlikely to be slower
> > on any conceivable hardware, but probably faster on most.
> >
> Do you have that hardware? I already objected versus table but do not
> have data. I wouldn't be surprised if its slower than byte-by-byte copy
> with if after each byte. Or just copy 8 bytes without condition but I am
> not sure how hardware handles overlapping stores. Difference will be
> bigger in practice, in profiling around 50% calls are 8 byte aligned and
> you save address calculation cost on these.

I think Richard's idea is good but I do agree with Steve that the tried and
tested code should go in first and then optimise it. There is lots of
exploration to do with MIPSR6 and there are many new ways to optimize. If
we don't have R6 support in glibc 2.21 then there is a definite performance
regression on R6 as the R5/R2 code will trap and emulate on an R6 core making
any non-trapping code several orders of magnitude better.

Overall we are trying to hit as many package release dates as possible to
provide everyone with initial R6 support for experimentation. For GLIBC that
not only includes all the specific R6 patches from Steve but also requires
the .MIPS.abiflags (FPXX/FP64 ABI) patch from myself.

Thanks,
Matthew
  

On Tue, Dec 23, 2014 at 11:15:02PM +0000, Matthew Fortune wrote:
> Ondřej Bílka <neleai@seznam.cz>  writes:
> > On Tue, Dec 23, 2014 at 09:52:56AM -0800, Richard Henderson wrote:
> > >
> > Do you have that hardware? I already objected versus table but do not
> > have data. I wouldn't be surprised if its slower than byte-by-byte copy
> > with if after each byte. Or just copy 8 bytes without condition but I am
> > not sure how hardware handles overlapping stores. Difference will be
> > bigger in practice, in profiling around 50% calls are 8 byte aligned and
> > you save address calculation cost on these.
> 
> I think Richard's idea is good but I do agree with Steve that the tried and
> tested code should go in first and then optimise it. There is lots of
> exploration to do with MIPSR6 and there are many new ways to optimize. If
> we don't have R6 support in glibc 2.21 then there is a definite performance
> regression on R6 as the R5/R2 code will trap and emulate on an R6 core making
> any non-trapping code several orders of magnitude better.
> 
> Overall we are trying to hit as many package release dates as possible to
> provide everyone with initial R6 support for experimentation. For GLIBC that
> not only includes all the specific R6 patches from Steve but also requires
> the .MIPS.abiflags (FPXX/FP64 ABI) patch from myself.
> 
That is valid argument. If that is objective then KISS when sending
patch as it will be easier to review and modify. When you try to add
optimizations you could expect comments that there is better way to
optimize it.
  

On Tue, 23 Dec 2014, Steve Ellcey wrote:

> +#if !defined (R6_CODE)

Just #ifndef.

> @@ -339,22 +426,22 @@ L(aligned):
>  	PREFETCH_FOR_STORE (3, a0)
>  #endif
>  #if defined(RETURN_FIRST_PREFETCH) && defined(USE_PREFETCH)
> -# if PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE
> +#if PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE
>  	sltu    v1,t9,a0
>  	bgtz    v1,L(skip_set)
>  	nop
>  	PTR_ADDIU v0,a0,(PREFETCH_CHUNK*4)
>  L(skip_set):
> -# else
> +#else
>  	PTR_ADDIU v0,a0,(PREFETCH_CHUNK*1)
> -# endif
> +#endif
>  #endif
>  #if defined(RETURN_LAST_PREFETCH) && defined(USE_PREFETCH) \
>      && (PREFETCH_STORE_HINT != PREFETCH_HINT_PREPAREFORSTORE)
>  	PTR_ADDIU v0,a0,(PREFETCH_CHUNK*3)
> -# ifdef USE_DOUBLE
> +#ifdef USE_DOUBLE
>  	PTR_ADDIU v0,v0,32
> -# endif
> +#endif
>  #endif
>  L(loop16w):
>  	C_LD	t0,UNIT(0)(a1)

These indentation changes seem wrong.

> @@ -363,8 +450,12 @@ L(loop16w):
>  	bgtz	v1,L(skip_pref)
>  #endif
>  	C_LD	t1,UNIT(1)(a1)
> +#if defined(R6_CODE)

Just #ifdef.

> +#if defined(R6_CODE)

> +#if !defined(R6_CODE)

> +#if !defined (R6_CODE)

Likewise.

> @@ -523,15 +622,15 @@ L(ua_chk16w):
>  	PREFETCH_FOR_STORE (3, a0)
>  #endif
>  #if defined(RETURN_FIRST_PREFETCH) && defined(USE_PREFETCH)
> -# if (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE)
> +#if (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE)
>  	sltu    v1,t9,a0
>  	bgtz    v1,L(ua_skip_set)
>  	nop
>  	PTR_ADDIU v0,a0,(PREFETCH_CHUNK*4)
>  L(ua_skip_set):
> -# else
> +#else
>  	PTR_ADDIU v0,a0,(PREFETCH_CHUNK*1)
> -# endif
> +#endif
>  #endif

More wrong indentation changes.