[patch, mips] Improved memset for MIPS

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[patch, mips] Improved memset for MIPS

Steve Ellcey -2
I would like to update the MIPS memset routine to include many of the
improvements I made to memcpy earlier.  These include better prefetching
and more loop unrolling for better performance.  Like with memset I use
ifdefs so it can be compiled in 32 or 64 bit modes and so I also remove
the old 64bit specific version of memset.S with this patch.

Tested with the glibc and gcc testsuites and by doing some standalone
performance measurements.

OK to checkin?

Steve Ellcey
[hidden email]



2013-09-05  Steve Ellcey  <[hidden email]>

        * sysdeps/mips/memset.S: Change prefetching and add loop unrolling.
        * sysdeps/mips/mips64/memset.S: Remove.



diff --git a/ports/sysdeps/mips/memset.S b/ports/sysdeps/mips/memset.S
index 85062fe..c7e5507 100644
--- a/ports/sysdeps/mips/memset.S
+++ b/ports/sysdeps/mips/memset.S
@@ -1,6 +1,5 @@
-/* Copyright (C) 2002-2013 Free Software Foundation, Inc.
+/* Copyright (C) 2013 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
-   Contributed by Hartvig Ekner <[hidden email]>, 2002.
 
    The GNU C Library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Lesser General Public
@@ -16,70 +15,357 @@
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
+#ifdef ANDROID_CHANGES
+#include "machine/asm.h"
+#include "machine/regdef.h"
+#define PREFETCH_STORE_HINT PREFETCH_HINT_PREPAREFORSTORE
+#elif _LIBC
 #include <sysdep.h>
+#include <regdef.h>
+#include <sys/asm.h>
+#define PREFETCH_STORE_HINT PREFETCH_HINT_PREPAREFORSTORE
+#elif _COMPILING_NEWLIB
+#include "machine/asm.h"
+#include "machine/regdef.h"
+#define PREFETCH_STORE_HINT PREFETCH_HINT_PREPAREFORSTORE
+#else
+#include <regdef.h>
+#include <sys/asm.h>
+#endif
 
- .set nomips16
+/* Check to see if the MIPS architecture we are compiling for supports
+ * prefetching.
+ */
+
+#if (__mips == 4) || (__mips == 5) || (__mips == 32) || (__mips == 64)
+#ifndef DISABLE_PREFETCH
+#define USE_PREFETCH
+#endif
+#endif
+
+#if defined(_MIPS_SIM) && ((_MIPS_SIM == _ABI64) || (_MIPS_SIM == _ABIN32))
+#ifndef DISABLE_DOUBLE
+#define USE_DOUBLE
+#endif
+#endif
+
+#ifndef USE_DOUBLE
+#ifndef DISABLE_DOUBLE_ALIGN
+#define DOUBLE_ALIGN
+#endif
+#endif
+
+/* Some asm.h files do not have the L macro definition.  */
+#ifndef L
+#if _MIPS_SIM == _ABIO32
+# define L(label) $L ## label
+#else
+# define L(label) .L ## label
+#endif
+#endif
+
+/* Some asm.h files do not have the PTR_ADDIU macro definition.  */
+#ifndef PTR_ADDIU
+#ifdef USE_DOUBLE
+#define PTR_ADDIU daddiu
+#else
+#define PTR_ADDIU addiu
+#endif
+#endif
+
+/*
+ * Using PREFETCH_HINT_PREPAREFORSTORE instead of PREFETCH_STORE
+ * or PREFETCH_STORE_STREAMED offers a large performance advantage
+ * but PREPAREFORSTORE has some special restrictions to consider.
+ *
+ * Prefetch with the 'prepare for store' hint does not copy a memory
+ * location into the cache, it just allocates a cache line and zeros
+ * it out.  This means that if you do not write to the entire cache
+ * line before writing it out to memory some data will get zero'ed out
+ * when the cache line is written back to memory and data will be lost.
+ *
+ * There are ifdef'ed sections of this memcpy to make sure that it does not
+ * do prefetches on cache lines that are not going to be completely written.
+ * This code is only needed and only used when PREFETCH_STORE_HINT is set to
+ * PREFETCH_HINT_PREPAREFORSTORE.  This code assumes that cache lines are
+ * less than MAX_PREFETCH_SIZE bytes and if the cache line is larger it will
+ * not work correctly.
+ */
+
+#ifdef USE_PREFETCH
+# define PREFETCH_HINT_STORE 1
+# define PREFETCH_HINT_STORE_STREAMED 5
+# define PREFETCH_HINT_STORE_RETAINED 7
+# define PREFETCH_HINT_PREPAREFORSTORE 30
+
+/*
+ * If we have not picked out what hints to use at this point use the
+ * standard load and store prefetch hints.
+ */
+#ifndef PREFETCH_STORE_HINT
+# define PREFETCH_STORE_HINT PREFETCH_HINT_STORE
+#endif
+
+/*
+ * We double everything when USE_DOUBLE is true so we do 2 prefetches to
+ * get 64 bytes in that case.  The assumption is that each individual
+ * prefetch brings in 32 bytes.
+ */
+#ifdef USE_DOUBLE
+# define PREFETCH_CHUNK 64
+# define PREFETCH_FOR_STORE(chunk, reg) \
+ pref PREFETCH_STORE_HINT, (chunk)*64(reg); \
+ pref PREFETCH_STORE_HINT, ((chunk)*64)+32(reg)
+#else
+# define PREFETCH_CHUNK 32
+# define PREFETCH_FOR_STORE(chunk, reg) \
+ pref PREFETCH_STORE_HINT, (chunk)*32(reg)
+#endif
+
+/* MAX_PREFETCH_SIZE is the maximum size of a prefetch, it must not be less
+ * than PREFETCH_CHUNK, the assumed size of each prefetch.  If the real size
+ * of a prefetch is greater than MAX_PREFETCH_SIZE and the PREPAREFORSTORE
+ * hint is used, the code will not work correctly.  If PREPAREFORSTORE is not
+ * used then MAX_PREFETCH_SIZE does not matter.  */
+#define MAX_PREFETCH_SIZE 128
+/* PREFETCH_LIMIT is set based on the fact that we never use an offset greater
+ * than 5 on a STORE prefetch and that a single prefetch can never be larger
+ * than MAX_PREFETCH_SIZE.  We add the extra 32 when USE_DOUBLE is set because
+ * we actually do two prefetches in that case, one 32 bytes after the other.  */
+#ifdef USE_DOUBLE
+# define PREFETCH_LIMIT (5 * PREFETCH_CHUNK) + 32 + MAX_PREFETCH_SIZE
+#else
+# define PREFETCH_LIMIT (5 * PREFETCH_CHUNK) + MAX_PREFETCH_SIZE
+#endif
+#if (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE) \
+    && ((PREFETCH_CHUNK * 4) < MAX_PREFETCH_SIZE)
+/* We cannot handle this because the initial prefetches may fetch bytes that
+ * are before the buffer being copied.  We start copies with an offset
+ * of 4 so avoid this situation when using PREPAREFORSTORE.  */
+#error "PREFETCH_CHUNK is too large and/or MAX_PREFETCH_SIZE is too small."
+#endif
+#else /* USE_PREFETCH not defined */
+# define PREFETCH_FOR_STORE(offset, reg)
+#endif
+
+/* Allow the routine to be named something else if desired.  */
+#ifndef MEMSET_NAME
+#define MEMSET_NAME memset
+#endif
 
-/* void *memset(void *s, int c, size_t n).  */
+/* We load/store 64 bits at a time when USE_DOUBLE is true.
+ * The C_ prefix stands for CHUNK and is used to avoid macro name
+ * conflicts with system header files.  */
 
+#ifdef USE_DOUBLE
+#  define C_ST sd
 #if __MIPSEB
-# define SWHI swl /* high part is left in big-endian */
+#  define C_STHI sdl /* high part is left in big-endian */
 #else
-# define SWHI swr /* high part is right in little-endian */
+#  define C_STHI sdr /* high part is right in little-endian */
+#endif
+#else
+#  define C_ST sw
+#if __MIPSEB
+#  define C_STHI swl /* high part is left in big-endian */
+#else
+#  define C_STHI swr /* high part is right in little-endian */
+#endif
 #endif
 
-ENTRY (memset)
+/* Bookkeeping values for 32 vs. 64 bit mode.  */
+#ifdef USE_DOUBLE
+#  define NSIZE 8
+#  define NSIZEMASK 0x3f
+#  define NSIZEDMASK 0x7f
+#else
+#  define NSIZE 4
+#  define NSIZEMASK 0x1f
+#  define NSIZEDMASK 0x3f
+#endif
+#define UNIT(unit) ((unit)*NSIZE)
+#define UNITM1(unit) (((unit)*NSIZE)-1)
+
+#ifdef ANDROID_CHANGES
+LEAF(MEMSET_NAME,0)
+#else
+LEAF(MEMSET_NAME)
+#endif
+
+ .set nomips16
  .set noreorder
+/*
+ * If the size is less than 2*NSIZE (8 or 16), go to L(lastb).  Regardless of
+ * size, copy dst pointer to v0 for the return value.
+ */
+ slti t2,a2,(2 * NSIZE)
+ bne t2,zero,L(lastb)
+ move v0,a0
 
- slti t1, a2, 8 # Less than 8?
- bne t1, zero, L(last8)
- move v0, a0 # Setup exit value before too late
-
- beq a1, zero, L(ueven) # If zero pattern, no need to extend
- andi a1, 0xff # Avoid problems with bogus arguments
- sll t0, a1, 8
- or a1, t0
- sll t0, a1, 16
- or a1, t0 # a1 is now pattern in full word
-
-L(ueven):
- subu t0, zero, a0 # Unaligned address?
- andi t0, 0x3
- beq t0, zero, L(chkw)
- subu a2, t0
- SWHI a1, 0(a0) # Yes, handle first unaligned part
- addu a0, t0 # Now both a0 and a2 are updated
+/*
+ * If memset value is not zero, we copy it to all the bytes in a 32 or 64
+ * bit word.
+ */
+ beq a1,zero,L(set0) /* If memset value is zero no smear  */
+ PTR_SUBU a3,zero,a0
+ nop
 
+ /* smear byte into 32 or 64 bit word */
+#if (__mips==32) && (__mips_isa_rev>=2)
+ ins     a1, a1, 8, 8        /* Replicate fill byte into half-word.  */
+ ins     a1, a1, 16, 16      /* Replicate fill byte into word.       */
+#ifdef USE_DOUBLE
+ dins a1, a1, 32, 32      /* Replicate fill byte into dbl word.   */
+#endif
+#else
+ sll t2,a1,8
+ or a1,t2
+ sll t2,a1,16
+ or a1,t2
+#ifdef USE_DOUBLE
+ dsll t2,a1,32
+ or a1,t2
+#endif
+#endif
+
+/*
+ * If the destination address is not aligned do a partial store to get it
+ * aligned.  If it is already aligned just jump to L(aligned).
+ */
+L(set0):
+ andi t2,a3,(NSIZE-1) /* word-unaligned address?          */
+ beq t2,zero,L(aligned) /* t2 is the unalignment count      */
+ PTR_SUBU a2,a2,t2
+ C_STHI a1,0(a0)
+ PTR_ADDU a0,a0,t2
+
+L(aligned):
+/*
+ * If USE_DOUBLE is not set we may still want to align the data on a 16
+ * byte boundry instead of an 8 byte boundry to maximize the opportunity
+ * of proAptive chips to do memory bonding (combining two sequential 4
+ * byte stores into one 8 byte store).  We know there are at least 4 bytes
+ * left to store or we would have jumped to L(lastb) earlier in the code.
+ */
+#ifdef DOUBLE_ALIGN
+ andi t2,a3,4
+ beq t2,zero,L(double_aligned)
+ PTR_SUBU a2,a2,t2
+ sw a1,0(a0)
+ PTR_ADDU a0,a0,t2
+L(double_aligned):
+#endif
+
+/*
+ * Now the destination is aligned to (word or double word) aligned address
+ * Set a2 to count how many bytes we have to copy after all the 64/128 byte
+ * chunks are copied and a3 to the dest pointer after all the 64/128 byte
+ * chunks have been copied.  We will loop, incrementing a0 until it equals a3.
+ */
+ andi t8,a2,NSIZEDMASK /* any whole 64-byte/128-byte chunks? */
+ beq a2,t8,L(chkw) /* if a2==t8, no 64-byte/128-byte chunks */
+ PTR_SUBU a3,a2,t8 /* subtract from a2 the reminder */
+ PTR_ADDU a3,a0,a3 /* Now a3 is the final dst after loop */
+
+/* When in the loop we may prefetch with the 'prepare to store' hint,
+ * in this case the a0+x should not be past the "t0-32" address.  This
+ * means: for x=128 the last "safe" a0 address is "t0-160".  Alternatively,
+ * for x=64 the last "safe" a0 address is "t0-96" In the current version we
+ * will use "prefetch hint,128(a0)", so "t0-160" is the limit.
+ */
+#if defined(USE_PREFETCH) && (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE)
+ PTR_ADDU t0,a0,a2 /* t0 is the "past the end" address */
+ PTR_SUBU t9,t0,PREFETCH_LIMIT /* t9 is the "last safe pref" address */
+ PREFETCH_FOR_STORE (1, a0)
+ PREFETCH_FOR_STORE (2, a0)
+ PREFETCH_FOR_STORE (3, a0)
+#endif
+L(loop16w):
+#if defined(USE_PREFETCH) && (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE)
+ sltu v1,t9,a0 /* If a0 > t9 don't use next prefetch */
+ bgtz v1,L(skip_pref)
+ nop
+#endif
+ PREFETCH_FOR_STORE (4, a0)
+ PREFETCH_FOR_STORE (5, a0)
+L(skip_pref):
+ C_ST a1,UNIT(0)(a0)
+ C_ST a1,UNIT(1)(a0)
+ C_ST a1,UNIT(2)(a0)
+ C_ST a1,UNIT(3)(a0)
+ C_ST a1,UNIT(4)(a0)
+ C_ST a1,UNIT(5)(a0)
+ C_ST a1,UNIT(6)(a0)
+ C_ST a1,UNIT(7)(a0)
+ C_ST a1,UNIT(8)(a0)
+ C_ST a1,UNIT(9)(a0)
+ C_ST a1,UNIT(10)(a0)
+ C_ST a1,UNIT(11)(a0)
+ C_ST a1,UNIT(12)(a0)
+ C_ST a1,UNIT(13)(a0)
+ C_ST a1,UNIT(14)(a0)
+ C_ST a1,UNIT(15)(a0)
+ PTR_ADDIU a0,a0,UNIT(16) /* adding 64/128 to dest */
+ bne a0,a3,L(loop16w)
+ nop
+ move a2,t8
+
+/*
+ * Here we have dest word-aligned but less than 64-bytes or 128 bytes to go.
+ * Check for a 32(64) byte chunk and copy if if there is one.  Otherwise
+ * jump down to L(chk1w) to handle the tail end of the copy.
+ */
 L(chkw):
- andi t0, a2, 0x7 # Enough left for one loop iteration?
- beq t0, a2, L(chkl)
- subu a3, a2, t0
- addu a3, a0 # a3 is last loop address +1
- move a2, t0 # a2 is now # of bytes left after loop
-L(loopw):
- addiu a0, 8 # Handle 2 words pr. iteration
- sw a1, -8(a0)
- bne a0, a3, L(loopw)
- sw a1, -4(a0)
-
-L(chkl):
- andi t0, a2, 0x4 # Check if there is at least a full
- beq t0, zero, L(last8) #  word remaining after the loop
- subu a2, t0
- sw a1, 0(a0) # Yes...
- addiu a0, 4
-
-L(last8):
- blez a2, L(exit) # Handle last 8 bytes (if cnt>0)
- addu a3, a2, a0 # a3 is last address +1
-L(lst8l):
- addiu a0, 1
- bne a0, a3, L(lst8l)
- sb a1, -1(a0)
-L(exit):
- j ra # Bye, bye
+ andi t8,a2,NSIZEMASK /* is there a 32-byte/64-byte chunk.  */
+ /* the t8 is the reminder count past 32-bytes */
+ beq a2,t8,L(chk1w)/* when a2==t8, no 32-byte chunk */
+ nop
+ C_ST a1,UNIT(0)(a0)
+ C_ST a1,UNIT(1)(a0)
+ C_ST a1,UNIT(2)(a0)
+ C_ST a1,UNIT(3)(a0)
+ C_ST a1,UNIT(4)(a0)
+ C_ST a1,UNIT(5)(a0)
+ C_ST a1,UNIT(6)(a0)
+ C_ST a1,UNIT(7)(a0)
+ PTR_ADDIU a0,a0,UNIT(8)
+
+/*
+ * Here we have less than 32(64) bytes to set.  Set up for a loop to
+ * copy one word (or double word) at a time.  Set a2 to count how many
+ * bytes we have to copy after all the word (or double word) chunks are
+ * copied and a3 to the dest pointer after all the (d)word chunks have
+ * been copied.  We will loop, incrementing a0 until a0 equals a3.
+ */
+L(chk1w):
+ andi a2,t8,(NSIZE-1) /* a2 is the reminder past one (d)word chunks */
+ beq a2,t8,L(lastb)
+ PTR_SUBU a3,t8,a2 /* a3 is count of bytes in one (d)word chunks */
+ PTR_ADDU a3,a0,a3 /* a3 is the dst address after loop */
+
+/* copying in words (4-byte or 8 byte chunks) */
+L(wordCopy_loop):
+ PTR_ADDIU a0,a0,UNIT(1)
+ bne a0,a3,L(wordCopy_loop)
+ C_ST a1,UNIT(-1)(a0)
+
+/* Copy the last 8 (or 16) bytes */
+L(lastb):
+ blez a2,L(leave)
+ PTR_ADDU a3,a0,a2       /* a3 is the last dst address */
+L(lastbloop):
+ PTR_ADDIU a0,a0,1
+ bne a0,a3,L(lastbloop)
+ sb a1,-1(a0)
+L(leave):
+ j ra
  nop
 
+ .set at
  .set reorder
-END (memset)
-libc_hidden_builtin_def (memset)
+END(MEMSET_NAME)
+#ifndef ANDROID_CHANGES
+#ifdef _LIBC
+libc_hidden_builtin_def (MEMSET_NAME)
+#endif
+#endif

Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Mike Frysinger
On Thursday 05 September 2013 13:05:43 Steve Ellcey wrote:
> --- a/ports/sysdeps/mips/memset.S
> +++ b/ports/sysdeps/mips/memset.S
> @@ -1,6 +1,5 @@
> -/* Copyright (C) 2002-2013 Free Software Foundation, Inc.
> +/* Copyright (C) 2013 Free Software Foundation, Inc.

err, that's not generally how it works ... we extend the years, but don't
delete them.

> -   Contributed by Hartvig Ekner <[hidden email]>, 2002.

what'd he ever do to you ? :p

> +#ifdef ANDROID_CHANGES

i wouldn't think we'd normally accept this kind of stuff, but i guess it's
already been done with the mips memcpy ...
-mike

signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Carlos O'Donell-6
In reply to this post by Steve Ellcey -2
On 09/05/2013 01:05 PM, Steve Ellcey wrote:
> I would like to update the MIPS memset routine to include many of the
> improvements I made to memcpy earlier.  These include better prefetching
> and more loop unrolling for better performance.  Like with memset I use
> ifdefs so it can be compiled in 32 or 64 bit modes and so I also remove
> the old 64bit specific version of memset.S with this patch.
>
> Tested with the glibc and gcc testsuites and by doing some standalone
> performance measurements.
> OK to checkin?

Two things really:

(a) Testing details?

Could you please elaborate more on "some standalone performance
measurements?"

What specific benchmarks did you run?

What does the glibc microbenchmark show about your changes? Do they
show a benefit?

Steve, I trust your experience with MIPS, but I'd like to see all
of us drive a little more detail into these performance related
patches. I'm also curious if the microbenchmark shows a performance
progression. The glibc community is trying hard to add some objectivity
to our performance measurements, prevent performance regressions, and
use the tests to experiment with new implementations.

(b) the code formatting isn't in line with the project requirements.

...

> 2013-09-05  Steve Ellcey  <[hidden email]>
>
> * sysdeps/mips/memset.S: Change prefetching and add loop unrolling.
> * sysdeps/mips/mips64/memset.S: Remove.
>
>
>
> diff --git a/ports/sysdeps/mips/memset.S b/ports/sysdeps/mips/memset.S
> index 85062fe..c7e5507 100644
> --- a/ports/sysdeps/mips/memset.S
> +++ b/ports/sysdeps/mips/memset.S
> @@ -1,6 +1,5 @@
> -/* Copyright (C) 2002-2013 Free Software Foundation, Inc.
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
> -   Contributed by Hartvig Ekner <[hidden email]>, 2002.
>  
>     The GNU C Library is free software; you can redistribute it and/or
>     modify it under the terms of the GNU Lesser General Public
> @@ -16,70 +15,357 @@
>     License along with the GNU C Library.  If not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#ifdef ANDROID_CHANGES
> +#include "machine/asm.h"
> +#include "machine/regdef.h"
> +#define PREFETCH_STORE_HINT PREFETCH_HINT_PREPAREFORSTORE
> +#elif _LIBC
>  #include <sysdep.h>
> +#include <regdef.h>
> +#include <sys/asm.h>
> +#define PREFETCH_STORE_HINT PREFETCH_HINT_PREPAREFORSTORE
> +#elif _COMPILING_NEWLIB
> +#include "machine/asm.h"
> +#include "machine/regdef.h"
> +#define PREFETCH_STORE_HINT PREFETCH_HINT_PREPAREFORSTORE
> +#else
> +#include <regdef.h>
> +#include <sys/asm.h>
> +#endif

This doesn't meet glibc's coding standards and you didn't provide any
rationale for not meeting the standards e.g. shared file amongst multiple
implementations.

See:
https://sourceware.org/glibc/wiki/Style_and_Conventions

Particularly:
https://sourceware.org/glibc/wiki/Style_and_Conventions#Nested_C_Preprocessor_Directives

I won't repeat this review comment for the other similarly formatted cpp defines.

> - .set nomips16
> +/* Check to see if the MIPS architecture we are compiling for supports
> + * prefetching.
> + */

These comments are not GNU style, they should be:

/* Line one.
   Line two.
   Line three.  */

I won't repeat this review comment for the other similarly formatted comments.

> +
> +#if (__mips == 4) || (__mips == 5) || (__mips == 32) || (__mips == 64)
> +#ifndef DISABLE_PREFETCH
> +#define USE_PREFETCH
> +#endif
> +#endif
> +
> +#if defined(_MIPS_SIM) && ((_MIPS_SIM == _ABI64) || (_MIPS_SIM == _ABIN32))
> +#ifndef DISABLE_DOUBLE
> +#define USE_DOUBLE
> +#endif
> +#endif
> +
> +#ifndef USE_DOUBLE
> +#ifndef DISABLE_DOUBLE_ALIGN
> +#define DOUBLE_ALIGN
> +#endif
> +#endif
> +
> +/* Some asm.h files do not have the L macro definition.  */
> +#ifndef L
> +#if _MIPS_SIM == _ABIO32
> +# define L(label) $L ## label
> +#else
> +# define L(label) .L ## label
> +#endif
> +#endif
> +
> +/* Some asm.h files do not have the PTR_ADDIU macro definition.  */
> +#ifndef PTR_ADDIU
> +#ifdef USE_DOUBLE
> +#define PTR_ADDIU daddiu
> +#else
> +#define PTR_ADDIU addiu
> +#endif
> +#endif
> +
> +/*
> + * Using PREFETCH_HINT_PREPAREFORSTORE instead of PREFETCH_STORE
> + * or PREFETCH_STORE_STREAMED offers a large performance advantage
> + * but PREPAREFORSTORE has some special restrictions to consider.
> + *
> + * Prefetch with the 'prepare for store' hint does not copy a memory
> + * location into the cache, it just allocates a cache line and zeros
> + * it out.  This means that if you do not write to the entire cache
> + * line before writing it out to memory some data will get zero'ed out
> + * when the cache line is written back to memory and data will be lost.
> + *
> + * There are ifdef'ed sections of this memcpy to make sure that it does not
> + * do prefetches on cache lines that are not going to be completely written.
> + * This code is only needed and only used when PREFETCH_STORE_HINT is set to
> + * PREFETCH_HINT_PREPAREFORSTORE.  This code assumes that cache lines are
> + * less than MAX_PREFETCH_SIZE bytes and if the cache line is larger it will
> + * not work correctly.
> + */
> +
> +#ifdef USE_PREFETCH
> +# define PREFETCH_HINT_STORE 1
> +# define PREFETCH_HINT_STORE_STREAMED 5
> +# define PREFETCH_HINT_STORE_RETAINED 7
> +# define PREFETCH_HINT_PREPAREFORSTORE 30

Not obvious that the endif for this is much later, which is why
cpp nesting is useful. You started using nesting here, but not
consistently.

> +
> +/*
> + * If we have not picked out what hints to use at this point use the
> + * standard load and store prefetch hints.
> + */
> +#ifndef PREFETCH_STORE_HINT
> +# define PREFETCH_STORE_HINT PREFETCH_HINT_STORE
> +#endif
> +
> +/*
> + * We double everything when USE_DOUBLE is true so we do 2 prefetches to
> + * get 64 bytes in that case.  The assumption is that each individual
> + * prefetch brings in 32 bytes.
> + */
> +#ifdef USE_DOUBLE
> +# define PREFETCH_CHUNK 64
> +# define PREFETCH_FOR_STORE(chunk, reg) \
> + pref PREFETCH_STORE_HINT, (chunk)*64(reg); \
> + pref PREFETCH_STORE_HINT, ((chunk)*64)+32(reg)
> +#else
> +# define PREFETCH_CHUNK 32
> +# define PREFETCH_FOR_STORE(chunk, reg) \
> + pref PREFETCH_STORE_HINT, (chunk)*32(reg)
> +#endif
> +
> +/* MAX_PREFETCH_SIZE is the maximum size of a prefetch, it must not be less
> + * than PREFETCH_CHUNK, the assumed size of each prefetch.  If the real size
> + * of a prefetch is greater than MAX_PREFETCH_SIZE and the PREPAREFORSTORE
> + * hint is used, the code will not work correctly.  If PREPAREFORSTORE is not
> + * used then MAX_PREFETCH_SIZE does not matter.  */
> +#define MAX_PREFETCH_SIZE 128
> +/* PREFETCH_LIMIT is set based on the fact that we never use an offset greater
> + * than 5 on a STORE prefetch and that a single prefetch can never be larger
> + * than MAX_PREFETCH_SIZE.  We add the extra 32 when USE_DOUBLE is set because
> + * we actually do two prefetches in that case, one 32 bytes after the other.  */
> +#ifdef USE_DOUBLE
> +# define PREFETCH_LIMIT (5 * PREFETCH_CHUNK) + 32 + MAX_PREFETCH_SIZE
> +#else
> +# define PREFETCH_LIMIT (5 * PREFETCH_CHUNK) + MAX_PREFETCH_SIZE
> +#endif
> +#if (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE) \
> +    && ((PREFETCH_CHUNK * 4) < MAX_PREFETCH_SIZE)
> +/* We cannot handle this because the initial prefetches may fetch bytes that
> + * are before the buffer being copied.  We start copies with an offset
> + * of 4 so avoid this situation when using PREPAREFORSTORE.  */
> +#error "PREFETCH_CHUNK is too large and/or MAX_PREFETCH_SIZE is too small."
> +#endif
> +#else /* USE_PREFETCH not defined */
> +# define PREFETCH_FOR_STORE(offset, reg)
> +#endif
> +
> +/* Allow the routine to be named something else if desired.  */
> +#ifndef MEMSET_NAME
> +#define MEMSET_NAME memset
> +#endif
>  
> -/* void *memset(void *s, int c, size_t n).  */
> +/* We load/store 64 bits at a time when USE_DOUBLE is true.
> + * The C_ prefix stands for CHUNK and is used to avoid macro name
> + * conflicts with system header files.  */
>  
> +#ifdef USE_DOUBLE
> +#  define C_ST sd

It should be one space for the nesting.

>  #if __MIPSEB
> -# define SWHI swl /* high part is left in big-endian */
> +#  define C_STHI sdl /* high part is left in big-endian */
>  #else
> -# define SWHI swr /* high part is right in little-endian */
> +#  define C_STHI sdr /* high part is right in little-endian */
> +#endif
> +#else
> +#  define C_ST sw
> +#if __MIPSEB
> +#  define C_STHI swl /* high part is left in big-endian */
> +#else
> +#  define C_STHI swr /* high part is right in little-endian */
> +#endif
>  #endif
>  
> -ENTRY (memset)
> +/* Bookkeeping values for 32 vs. 64 bit mode.  */
> +#ifdef USE_DOUBLE
> +#  define NSIZE 8
> +#  define NSIZEMASK 0x3f
> +#  define NSIZEDMASK 0x7f
> +#else
> +#  define NSIZE 4
> +#  define NSIZEMASK 0x1f
> +#  define NSIZEDMASK 0x3f
> +#endif
> +#define UNIT(unit) ((unit)*NSIZE)
> +#define UNITM1(unit) (((unit)*NSIZE)-1)
> +
> +#ifdef ANDROID_CHANGES
> +LEAF(MEMSET_NAME,0)
> +#else
> +LEAF(MEMSET_NAME)
> +#endif
> +
> + .set nomips16
>   .set noreorder
> +/*
> + * If the size is less than 2*NSIZE (8 or 16), go to L(lastb).  Regardless of
> + * size, copy dst pointer to v0 for the return value.
> + */
> + slti t2,a2,(2 * NSIZE)
> + bne t2,zero,L(lastb)
> + move v0,a0
>  
> - slti t1, a2, 8 # Less than 8?
> - bne t1, zero, L(last8)
> - move v0, a0 # Setup exit value before too late
> -
> - beq a1, zero, L(ueven) # If zero pattern, no need to extend
> - andi a1, 0xff # Avoid problems with bogus arguments
> - sll t0, a1, 8
> - or a1, t0
> - sll t0, a1, 16
> - or a1, t0 # a1 is now pattern in full word
> -
> -L(ueven):
> - subu t0, zero, a0 # Unaligned address?
> - andi t0, 0x3
> - beq t0, zero, L(chkw)
> - subu a2, t0
> - SWHI a1, 0(a0) # Yes, handle first unaligned part
> - addu a0, t0 # Now both a0 and a2 are updated
> +/*
> + * If memset value is not zero, we copy it to all the bytes in a 32 or 64
> + * bit word.
> + */
> + beq a1,zero,L(set0) /* If memset value is zero no smear  */
> + PTR_SUBU a3,zero,a0
> + nop
>  
> + /* smear byte into 32 or 64 bit word */
> +#if (__mips==32) && (__mips_isa_rev>=2)
> + ins     a1, a1, 8, 8        /* Replicate fill byte into half-word.  */
> + ins     a1, a1, 16, 16      /* Replicate fill byte into word.       */
> +#ifdef USE_DOUBLE
> + dins a1, a1, 32, 32      /* Replicate fill byte into dbl word.   */
> +#endif
> +#else
> + sll t2,a1,8
> + or a1,t2
> + sll t2,a1,16
> + or a1,t2
> +#ifdef USE_DOUBLE
> + dsll t2,a1,32
> + or a1,t2
> +#endif
> +#endif
> +
> +/*
> + * If the destination address is not aligned do a partial store to get it
> + * aligned.  If it is already aligned just jump to L(aligned).
> + */
> +L(set0):
> + andi t2,a3,(NSIZE-1) /* word-unaligned address?          */
> + beq t2,zero,L(aligned) /* t2 is the unalignment count      */
> + PTR_SUBU a2,a2,t2
> + C_STHI a1,0(a0)
> + PTR_ADDU a0,a0,t2
> +
> +L(aligned):
> +/*
> + * If USE_DOUBLE is not set we may still want to align the data on a 16
> + * byte boundry instead of an 8 byte boundry to maximize the opportunity
> + * of proAptive chips to do memory bonding (combining two sequential 4
> + * byte stores into one 8 byte store).  We know there are at least 4 bytes
> + * left to store or we would have jumped to L(lastb) earlier in the code.
> + */
> +#ifdef DOUBLE_ALIGN
> + andi t2,a3,4
> + beq t2,zero,L(double_aligned)
> + PTR_SUBU a2,a2,t2
> + sw a1,0(a0)
> + PTR_ADDU a0,a0,t2
> +L(double_aligned):
> +#endif
> +
> +/*
> + * Now the destination is aligned to (word or double word) aligned address
> + * Set a2 to count how many bytes we have to copy after all the 64/128 byte
> + * chunks are copied and a3 to the dest pointer after all the 64/128 byte
> + * chunks have been copied.  We will loop, incrementing a0 until it equals a3.
> + */
> + andi t8,a2,NSIZEDMASK /* any whole 64-byte/128-byte chunks? */
> + beq a2,t8,L(chkw) /* if a2==t8, no 64-byte/128-byte chunks */
> + PTR_SUBU a3,a2,t8 /* subtract from a2 the reminder */
> + PTR_ADDU a3,a0,a3 /* Now a3 is the final dst after loop */
> +
> +/* When in the loop we may prefetch with the 'prepare to store' hint,
> + * in this case the a0+x should not be past the "t0-32" address.  This
> + * means: for x=128 the last "safe" a0 address is "t0-160".  Alternatively,
> + * for x=64 the last "safe" a0 address is "t0-96" In the current version we
> + * will use "prefetch hint,128(a0)", so "t0-160" is the limit.
> + */
> +#if defined(USE_PREFETCH) && (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE)
> + PTR_ADDU t0,a0,a2 /* t0 is the "past the end" address */
> + PTR_SUBU t9,t0,PREFETCH_LIMIT /* t9 is the "last safe pref" address */
> + PREFETCH_FOR_STORE (1, a0)
> + PREFETCH_FOR_STORE (2, a0)
> + PREFETCH_FOR_STORE (3, a0)
> +#endif
> +L(loop16w):
> +#if defined(USE_PREFETCH) && (PREFETCH_STORE_HINT == PREFETCH_HINT_PREPAREFORSTORE)
> + sltu v1,t9,a0 /* If a0 > t9 don't use next prefetch */
> + bgtz v1,L(skip_pref)
> + nop
> +#endif
> + PREFETCH_FOR_STORE (4, a0)
> + PREFETCH_FOR_STORE (5, a0)
> +L(skip_pref):
> + C_ST a1,UNIT(0)(a0)
> + C_ST a1,UNIT(1)(a0)
> + C_ST a1,UNIT(2)(a0)
> + C_ST a1,UNIT(3)(a0)
> + C_ST a1,UNIT(4)(a0)
> + C_ST a1,UNIT(5)(a0)
> + C_ST a1,UNIT(6)(a0)
> + C_ST a1,UNIT(7)(a0)
> + C_ST a1,UNIT(8)(a0)
> + C_ST a1,UNIT(9)(a0)
> + C_ST a1,UNIT(10)(a0)
> + C_ST a1,UNIT(11)(a0)
> + C_ST a1,UNIT(12)(a0)
> + C_ST a1,UNIT(13)(a0)
> + C_ST a1,UNIT(14)(a0)
> + C_ST a1,UNIT(15)(a0)
> + PTR_ADDIU a0,a0,UNIT(16) /* adding 64/128 to dest */
> + bne a0,a3,L(loop16w)
> + nop
> + move a2,t8
> +
> +/*
> + * Here we have dest word-aligned but less than 64-bytes or 128 bytes to go.
> + * Check for a 32(64) byte chunk and copy if if there is one.  Otherwise
> + * jump down to L(chk1w) to handle the tail end of the copy.
> + */
>  L(chkw):
> - andi t0, a2, 0x7 # Enough left for one loop iteration?
> - beq t0, a2, L(chkl)
> - subu a3, a2, t0
> - addu a3, a0 # a3 is last loop address +1
> - move a2, t0 # a2 is now # of bytes left after loop
> -L(loopw):
> - addiu a0, 8 # Handle 2 words pr. iteration
> - sw a1, -8(a0)
> - bne a0, a3, L(loopw)
> - sw a1, -4(a0)
> -
> -L(chkl):
> - andi t0, a2, 0x4 # Check if there is at least a full
> - beq t0, zero, L(last8) #  word remaining after the loop
> - subu a2, t0
> - sw a1, 0(a0) # Yes...
> - addiu a0, 4
> -
> -L(last8):
> - blez a2, L(exit) # Handle last 8 bytes (if cnt>0)
> - addu a3, a2, a0 # a3 is last address +1
> -L(lst8l):
> - addiu a0, 1
> - bne a0, a3, L(lst8l)
> - sb a1, -1(a0)
> -L(exit):
> - j ra # Bye, bye
> + andi t8,a2,NSIZEMASK /* is there a 32-byte/64-byte chunk.  */
> + /* the t8 is the reminder count past 32-bytes */
> + beq a2,t8,L(chk1w)/* when a2==t8, no 32-byte chunk */
> + nop
> + C_ST a1,UNIT(0)(a0)
> + C_ST a1,UNIT(1)(a0)
> + C_ST a1,UNIT(2)(a0)
> + C_ST a1,UNIT(3)(a0)
> + C_ST a1,UNIT(4)(a0)
> + C_ST a1,UNIT(5)(a0)
> + C_ST a1,UNIT(6)(a0)
> + C_ST a1,UNIT(7)(a0)
> + PTR_ADDIU a0,a0,UNIT(8)
> +
> +/*
> + * Here we have less than 32(64) bytes to set.  Set up for a loop to
> + * copy one word (or double word) at a time.  Set a2 to count how many
> + * bytes we have to copy after all the word (or double word) chunks are
> + * copied and a3 to the dest pointer after all the (d)word chunks have
> + * been copied.  We will loop, incrementing a0 until a0 equals a3.
> + */
> +L(chk1w):
> + andi a2,t8,(NSIZE-1) /* a2 is the reminder past one (d)word chunks */
> + beq a2,t8,L(lastb)
> + PTR_SUBU a3,t8,a2 /* a3 is count of bytes in one (d)word chunks */
> + PTR_ADDU a3,a0,a3 /* a3 is the dst address after loop */
> +
> +/* copying in words (4-byte or 8 byte chunks) */
> +L(wordCopy_loop):
> + PTR_ADDIU a0,a0,UNIT(1)
> + bne a0,a3,L(wordCopy_loop)
> + C_ST a1,UNIT(-1)(a0)
> +
> +/* Copy the last 8 (or 16) bytes */
> +L(lastb):
> + blez a2,L(leave)
> + PTR_ADDU a3,a0,a2       /* a3 is the last dst address */
> +L(lastbloop):
> + PTR_ADDIU a0,a0,1
> + bne a0,a3,L(lastbloop)
> + sb a1,-1(a0)
> +L(leave):
> + j ra
>   nop
>  
> + .set at
>   .set reorder
> -END (memset)
> -libc_hidden_builtin_def (memset)
> +END(MEMSET_NAME)
> +#ifndef ANDROID_CHANGES
> +#ifdef _LIBC
> +libc_hidden_builtin_def (MEMSET_NAME)
> +#endif
> +#endif
>

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Joseph Myers
In reply to this post by Steve Ellcey -2
On Thu, 5 Sep 2013, Steve Ellcey  wrote:

> Tested with the glibc and gcc testsuites and by doing some standalone
> performance measurements.

Has the glibc testsuite been run without regressions for all six
combinations of (o32, n32, n64) with (big-endian, little-endian)?

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Mike Frysinger
On Thu, 2013-09-05 at 20:40 -0400, Mike Frysinger wrote:
> On Thursday 05 September 2013 13:05:43 Steve Ellcey wrote:
> > --- a/ports/sysdeps/mips/memset.S
> > +++ b/ports/sysdeps/mips/memset.S
> > @@ -1,6 +1,5 @@
> > -/* Copyright (C) 2002-2013 Free Software Foundation, Inc.
> > +/* Copyright (C) 2013 Free Software Foundation, Inc.
>
> err, that's not generally how it works ... we extend the years, but don't
> delete them.

I am replaced the entire file.  While the diff makes it look like some
stuff is not replaced, the standard copyright text is about the only
thing not completely changed.  There are some blank lines and maybe a
couple of assembly language psuedo-ops that happen to match up by chance
but that is pure coincidence.

>
> > -   Contributed by Hartvig Ekner <[hidden email]>, 2002.
>
> what'd he ever do to you ? :p

Nothing, but I didn't use any of the old code.

> > +#ifdef ANDROID_CHANGES
>
> i wouldn't think we'd normally accept this kind of stuff, but i guess it's
> already been done with the mips memcpy ...
> -mike

Yes, I am trying to have a common memset, memcpy, and maybe some other
routines between glibc, newlib, and android/bionic.

Steve Ellcey
[hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Joseph Myers
On Fri, 2013-09-06 at 14:30 +0000, Joseph S. Myers wrote:
> On Thu, 5 Sep 2013, Steve Ellcey  wrote:
>
> > Tested with the glibc and gcc testsuites and by doing some standalone
> > performance measurements.
>
> Has the glibc testsuite been run without regressions for all six
> combinations of (o32, n32, n64) with (big-endian, little-endian)?

No.  I did most of my testing outside of the glibc testsuite because I
find the glibc testsuite difficult to run, see
https://sourceware.org/ml/libc-help/2013-08/msg00040.html for some of my
problems/questions.  I don't believe I have ever managed to do a clean
glibc testsuite run even in a clean tree with no local changes.  I did a
glibc testsuite run in o32 little-endian mode and did not see any
failures that looked like regressions from what I saw with a clean tree.
I did the rest of my testing outside of that.

Steve Ellcey
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Carlos O'Donell-6
On Fri, 2013-09-06 at 00:18 -0400, Carlos O'Donell wrote:

> Two things really:
>
> (a) Testing details?
>
> Could you please elaborate more on "some standalone performance
> measurements?"
>
> What specific benchmarks did you run?

Basically, I just wrote and used a test program that does a bunch of
memset's.  Nothing fancy or very intricate.

> What does the glibc microbenchmark show about your changes? Do they
> show a benefit?

I didn't try this, but I can.  Is there anything on the glibc web page
about how to run this benchmark?  Does it happen as part of the standard
'make check'?

>
> Steve, I trust your experience with MIPS, but I'd like to see all
> of us drive a little more detail into these performance related
> patches. I'm also curious if the microbenchmark shows a performance
> progression. The glibc community is trying hard to add some objectivity
> to our performance measurements, prevent performance regressions, and
> use the tests to experiment with new implementations.

That sounds reasonable.  I just need a bit of help on where this is and
how to run it.

> (b) the code formatting isn't in line with the project requirements.

I'll fix these up and resubmit when I have the changes (and some more
performance data).

Steve Ellcey
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Joseph Myers
In reply to this post by Steve Ellcey -2
On Fri, 6 Sep 2013, Steve Ellcey wrote:

> On Fri, 2013-09-06 at 14:30 +0000, Joseph S. Myers wrote:
> > On Thu, 5 Sep 2013, Steve Ellcey  wrote:
> >
> > > Tested with the glibc and gcc testsuites and by doing some standalone
> > > performance measurements.
> >
> > Has the glibc testsuite been run without regressions for all six
> > combinations of (o32, n32, n64) with (big-endian, little-endian)?
>
> No.  I did most of my testing outside of the glibc testsuite because I
> find the glibc testsuite difficult to run, see
> https://sourceware.org/ml/libc-help/2013-08/msg00040.html for some of my
> problems/questions.  I don't believe I have ever managed to do a clean

You'll need to debug the problems as they indicate something wrong with
your build environment.  It's always advised to configure glibc with
--prefix=/usr rather than some other prefix (but there is no requirement
that the dynamic linker actually be installed during testing, you can
ignore the -dynamic-linker= path), and your other error indicates some
inconsistency regarding NO_CTORS_DTORS_SECTIONS.

If you see more failures than are described at
<https://sourceware.org/glibc/wiki/Release/2.18>, you should investigate
them as well.

The expectation is that the glibc testsuite is the normal way to test
patches before submission, and string function patches like this need it
to be run for all six relevant ABI variants.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
On Fri, 2013-09-06 at 16:09 +0000, Joseph S. Myers wrote:

> > No.  I did most of my testing outside of the glibc testsuite because I
> > find the glibc testsuite difficult to run, see
> > https://sourceware.org/ml/libc-help/2013-08/msg00040.html for some of my
> > problems/questions.  I don't believe I have ever managed to do a clean
>
> You'll need to debug the problems as they indicate something wrong with
> your build environment.  It's always advised to configure glibc with
> --prefix=/usr rather than some other prefix (but there is no requirement
> that the dynamic linker actually be installed during testing, you can
> ignore the -dynamic-linker= path), and your other error indicates some
> inconsistency regarding NO_CTORS_DTORS_SECTIONS.

I have found that --prefix=/usr is more of a problem then a help when
building general cross compiler toolchains.  Using a prefix of /usr
triggers various special case code in
ports/sysdeps/unix/sysv/linux/mips/configure to put things in lib32 and
lib64 and I don't actually want any of that so I use a prefix
of /usr/fake instead of /usr.

The "undefined reference to `__libc_global_ctors'" has just shown up
again in a parallel build, but it seems to go away when I rebuild.  I am
still trying to understand what is going on with this.

> The expectation is that the glibc testsuite is the normal way to test
> patches before submission, and string function patches like this need it
> to be run for all six relevant ABI variants.

I think some flexibility here would be better.  There is no floating
point code in this routine so running all three ABI's in both hard and
soft float modes seems like overkill to me.  Testing in big and little
endian modes would seem more likely to turn up problems then testing in
hard and soft float.

Steve Ellcey
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Joseph Myers
On Fri, 2013-09-06 at 16:09 +0000, Joseph S. Myers wrote:

> The expectation is that the glibc testsuite is the normal way to test
> patches before submission, and string function patches like this need it
> to be run for all six relevant ABI variants.

My last email mentioned hard and soft float, but you were obviously
refering to big and little endian here as you said earlier in the email.
Sorry about that, I got confused in my reply.

Steve Ellcey
[hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Joseph Myers
In reply to this post by Steve Ellcey -2
On Fri, 6 Sep 2013, Steve Ellcey wrote:

> I have found that --prefix=/usr is more of a problem then a help when
> building general cross compiler toolchains.  Using a prefix of /usr
> triggers various special case code in
> ports/sysdeps/unix/sysv/linux/mips/configure to put things in lib32 and
> lib64 and I don't actually want any of that so I use a prefix
> of /usr/fake instead of /usr.

Not using --prefix=/usr runs into ABI testsuite problems with bug 14664.

> The "undefined reference to `__libc_global_ctors'" has just shown up
> again in a parallel build, but it seems to go away when I rebuild.  I am
> still trying to understand what is going on with this.

Since it seems to be about parallel builds and linkobj/libc.so, try with
Brooks's patch
<https://sourceware.org/ml/libc-alpha/2013-08/msg00597.html>?  (Which, if
it works OK on master for a while, should probably be backported to 2.18
branch.)

> > The expectation is that the glibc testsuite is the normal way to test
> > patches before submission, and string function patches like this need it
> > to be run for all six relevant ABI variants.
>
> I think some flexibility here would be better.  There is no floating
> point code in this routine so running all three ABI's in both hard and
> soft float modes seems like overkill to me.  Testing in big and little
> endian modes would seem more likely to turn up problems then testing in
> hard and soft float.

That's why I said six rather than twelve ABI variants; floating-point
variants aren't relevant here, but the other variants are.  You could
argue for full testing for three variants that cover all of o32, n32 and
n64 and both BE and LE, plus just the string/ directory tests for the
other three variants, but I think that would be the minimum.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Carlos O'Donell-6
In reply to this post by Steve Ellcey -2
On 09/06/2013 12:03 PM, Steve Ellcey wrote:

> On Fri, 2013-09-06 at 00:18 -0400, Carlos O'Donell wrote:
>
>> Two things really:
>>
>> (a) Testing details?
>>
>> Could you please elaborate more on "some standalone performance
>> measurements?"
>>
>> What specific benchmarks did you run?
>
> Basically, I just wrote and used a test program that does a bunch of
> memset's.  Nothing fancy or very intricate.

Are you able to post this test program for posterity along with
your patches?

>> What does the glibc microbenchmark show about your changes? Do they
>> show a benefit?
>
> I didn't try this, but I can.  Is there anything on the glibc web page
> about how to run this benchmark?  Does it happen as part of the standard
> 'make check'?

Just run `make bench', wait a while, and compare results before and after.

Look at bench/README for more details.

>>
>> Steve, I trust your experience with MIPS, but I'd like to see all
>> of us drive a little more detail into these performance related
>> patches. I'm also curious if the microbenchmark shows a performance
>> progression. The glibc community is trying hard to add some objectivity
>> to our performance measurements, prevent performance regressions, and
>> use the tests to experiment with new implementations.
>
> That sounds reasonable.  I just need a bit of help on where this is and
> how to run it.

My pleasure. Ask if you get stuck.

>> (b) the code formatting isn't in line with the project requirements.
>
> I'll fix these up and resubmit when I have the changes (and some more
> performance data).

Thanks for being accommodating.

Cheers,
Carlos.


Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Joseph Myers
On Fri, 2013-09-06 at 16:59 +0000, Joseph S. Myers wrote:

> Since it seems to be about parallel builds and linkobj/libc.so, try with
> Brooks's patch
> <https://sourceware.org/ml/libc-alpha/2013-08/msg00597.html>?  (Which, if
> it works OK on master for a while, should probably be backported to 2.18
> branch.)

I'll try this patch out.  Your comment about working OK on master made
me think it was already on ToT but that does not seem to be the case.
The patch no longer applies cleanly to ToT due to other
Makefile/Makerule changes, I tried to apply it by hand and I hope it
works.  Unfortunately, the problem I was seeing was sporadic so I may
not immediately know.

Steve Ellcey


Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Brooks Moses-5
On Fri, Sep 6, 2013 at 10:43 AM, Steve Ellcey <[hidden email]> wrote:

> On Fri, 2013-09-06 at 16:59 +0000, Joseph S. Myers wrote:
>>> The "undefined reference to `__libc_global_ctors'" has just shown up
>>> again in a parallel build, but it seems to go away when I rebuild.  I am
>>> still trying to understand what is going on with this.
>
>> Since it seems to be about parallel builds and linkobj/libc.so, try with
>> Brooks's patch
>> <https://sourceware.org/ml/libc-alpha/2013-08/msg00597.html>?  (Which, if
>> it works OK on master for a while, should probably be backported to 2.18
>> branch.)

Joseph is correct that this is related -- that error message is
exactly the one we were getting that sent me on the path of debugging
the circular reference and writing that patch.  The compile
instruction that causes that error message is consistent with the
implicit rule that my patch fixes.

- Brooks
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Carlos O'Donell-6
On Fri, 2013-09-06 at 13:12 -0400, Carlos O'Donell wrote:

> Are you able to post this test program for posterity along with
> your patches?

I have attached it to this email.  I compile with -UVERIFY when doing
benchmarks and with -DVERIFY when I am doing correctness testing.  On
one of my 74k boards the old memset took 63.409 seconds and the new one
took 45.577 seconds.  I played with different prefetch hints too while
benchmarking but the prepare-to-store one is the fastest.


> Just run `make bench', wait a while, and compare results before and after.
>
> Look at bench/README for more details.

I tried running it but all the tests failed with messages like this:


Running /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/benchtests/bench-bcopy
/home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: 1: /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: ^?ELF^A^A^A^C^H^A�^O4~\�: not found
/home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: 2: /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: Syntax error: "(" unexpected


I am not quite sure what to make of this, it seems to be using the right
ld.so.1 but I am not sure what it is that is 'not found'  Could this be
related to the issue of installing the latest libgcc and libstdc++ in
default locations? (glibc 2.18 wiki section 5.1.1)  I built glibc with a
GCC from a non-standard location so the libgcc and libstdc++ for that
compiler are not in the standard locations.

Steve Ellcey
[hidden email]


test_memset.c (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Carlos O'Donell-6
On 09/06/2013 07:33 PM, Steve Ellcey wrote:

> On Fri, 2013-09-06 at 13:12 -0400, Carlos O'Donell wrote:
>
>> Are you able to post this test program for posterity along with
>> your patches?
>
> I have attached it to this email.  I compile with -UVERIFY when doing
> benchmarks and with -DVERIFY when I am doing correctness testing.  On
> one of my 74k boards the old memset took 63.409 seconds and the new one
> took 45.577 seconds.  I played with different prefetch hints too while
> benchmarking but the prepare-to-store one is the fastest.

Thanks for sharing that.
 
>> Just run `make bench', wait a while, and compare results before and after.
>>
>> Look at bench/README for more details.
>
> I tried running it but all the tests failed with messages like this:
>
> Running /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/benchtests/bench-bcopy
> /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: 1: /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: ^?ELF^A^A^A^C^H^A�^O4~\�: not found
> /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: 2: /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: Syntax error: "(" unexpected

That's really quite odd.

Have the Makefile print what it's going to run and then re-run it by hand?

e.g.

diff --git a/benchtests/Makefile b/benchtests/Makefile
index 4d4b909..b0f0716 100644
--- a/benchtests/Makefile
+++ b/benchtests/Makefile
@@ -146,7 +146,7 @@ bench: bench-set bench-func
 
 bench-set: $(binaries-benchset)
        for run in $^; do \
-         echo "Running $${run}"; \
+         echo "Running $(run-bench)"; \
          $(run-bench) > $${run}.out; \
        done
---
 
> I am not quite sure what to make of this, it seems to be using the right
> ld.so.1 but I am not sure what it is that is 'not found'  Could this be
> related to the issue of installing the latest libgcc and libstdc++ in
> default locations? (glibc 2.18 wiki section 5.1.1)  I built glibc with a
> GCC from a non-standard location so the libgcc and libstdc++ for that
> compiler are not in the standard locations.

No, libgcc won't matter unless you do cancellation, and libstdc++ doesn't
matter because it's not a C++ application. You'll just get glibc using
the versions of those from the related prefix directories. I don't think
it should be making any difference here.

You've certainly got your share of weird environment issues :-)

Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Andreas Schwab-2
In reply to this post by Steve Ellcey -2
Steve Ellcey <[hidden email]> writes:

> Running /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/benchtests/bench-bcopy
> /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: 1: /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: ^?ELF^A^A^A^C^H^A�^O4~\�: not found
> /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: 2: /home/sellcey/gcc/memset/obj-mipsisa32r2el-linux-gnu/glibc/obj_default/elf/ld.so.1: Syntax error: "(" unexpected
>
>
> I am not quite sure what to make of this,

This means that the binary format is not executable, and the shell tries
to interpret it as a script.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
In reply to this post by Carlos O'Donell-6
On Fri, 2013-09-06 at 22:38 -0400, Carlos O'Donell wrote:

> No, libgcc won't matter unless you do cancellation, and libstdc++ doesn't
> matter because it's not a C++ application. You'll just get glibc using
> the versions of those from the related prefix directories. I don't think
> it should be making any difference here.
>
> You've certainly got your share of weird environment issues :-)
>
> Cheers,
> Carlos.

As an FYI, I think I have figured out my testing problems.  I normally
build cross toolchains by building binutils; gcc (using
--without-headers); glibc; then a final GCC.  When building on a MIPS
machine I was doing the same thing and then going back to the glibc
object directory and running 'make check' or 'make bench'.  I think the
problem with this was that the GCC now in my path (the final GCC) is not
the same GCC that I used to build glibc (the initial --without-headers
GCC).  This seemed to trigger a partial rebuild of glibc along with
building the tests and that in turn caused all sorts of weird problems.

If I build the toolchain, then build a new glibc using the final GCC and
run 'make check' or 'make bench' in that glibc object directory I get
just the expected MIPS failures.

Now that I can see the results of 'make bench' I do have a question,
what is the difference between the results in bench-memset.out and
bench-memset-ifunc.out?  MIPS doesn't yet support IFUNC.  It looks like
the results in the two files are pretty close, so maybe they are
identical runs on machines with no IFUNC?

Steve Ellcey
[hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Carlos O'Donell-6
On 09/10/2013 04:29 PM, Steve Ellcey wrote:

> On Fri, 2013-09-06 at 22:38 -0400, Carlos O'Donell wrote:
>
>> No, libgcc won't matter unless you do cancellation, and libstdc++ doesn't
>> matter because it's not a C++ application. You'll just get glibc using
>> the versions of those from the related prefix directories. I don't think
>> it should be making any difference here.
>>
>> You've certainly got your share of weird environment issues :-)
>>
>> Cheers,
>> Carlos.
>
> As an FYI, I think I have figured out my testing problems.  I normally
> build cross toolchains by building binutils; gcc (using
> --without-headers); glibc; then a final GCC.  When building on a MIPS
> machine I was doing the same thing and then going back to the glibc
> object directory and running 'make check' or 'make bench'.  I think the
> problem with this was that the GCC now in my path (the final GCC) is not
> the same GCC that I used to build glibc (the initial --without-headers
> GCC).  This seemed to trigger a partial rebuild of glibc along with
> building the tests and that in turn caused all sorts of weird problems.
>
> If I build the toolchain, then build a new glibc using the final GCC and
> run 'make check' or 'make bench' in that glibc object directory I get
> just the expected MIPS failures.
>
> Now that I can see the results of 'make bench' I do have a question,
> what is the difference between the results in bench-memset.out and
> bench-memset-ifunc.out?  MIPS doesn't yet support IFUNC.  It looks like
> the results in the two files are pretty close, so maybe they are
> identical runs on machines with no IFUNC?

You get the default implementation of __libc_ifunc_impl_list (the function
used by the testing infrastructure to iterate the functions implemented
as ifuncs) which adds no additional functions to the test list. You still
test the usual defaults e.g. simple, builtin, and original function entry.
Therefore it's the same as the non-IFUNC version with the results being
the same modulo testing variance.

Does that answer your question?

Cheers,
Carlos.

Reply | Threaded
Open this post in threaded view
|

Re: [patch, mips] Improved memset for MIPS

Steve Ellcey -2
On Tue, 2013-09-10 at 17:01 -0400, Carlos O'Donell wrote:

> > Now that I can see the results of 'make bench' I do have a question,
> > what is the difference between the results in bench-memset.out and
> > bench-memset-ifunc.out?  MIPS doesn't yet support IFUNC.  It looks like
> > the results in the two files are pretty close, so maybe they are
> > identical runs on machines with no IFUNC?
>
> You get the default implementation of __libc_ifunc_impl_list (the function
> used by the testing infrastructure to iterate the functions implemented
> as ifuncs) which adds no additional functions to the test list. You still
> test the usual defaults e.g. simple, builtin, and original function entry.
> Therefore it's the same as the non-IFUNC version with the results being
> the same modulo testing variance.
>
> Does that answer your question?

I think so, but just to be clear: If I did have IFUNC and 4 different
implementations of memset (for example), would the testing
infrastructure run and benchmark all 4 versions of memset?

Steve Ellcey



12