[PATCH] more problems with newlib/libc/machine/m68k/memcpy.S

[Josef Wolf <jw at raven dot inka dot de>]

Hello folks,

I have done a closer look at newlib/libc/machine/m68k/memcpy.S and I think
there are a couple of problems with this code:

1. When the destination pointer is mis-aligned, up to three bytes are copied
   before getting into the optimized main loop. But d1, which contains the
   number of bytes to copy, is never adjusted to account for this fact. As a
   consequence, up to 3 byte too much can be copied by memcpy().

2. When MISALIGNED_OK==0, no attempt is made to get the pointers to aligned
   positions. This results in much slower code than the old code for such
   CPUs. IMHO, the cause of the problem reported in the thread starting with
   http://sourceware.org/ml/newlib/2009/msg01079.html was mis-interpreted.
   The problem was _not_ the attempt to align dest. The problem was that
   even after dest was aligned, src was still not aligned. Thus, disabling
   alignment completely is the wrong response to that problem.

3. -mcpu32 seems to imply -mc68020. So the check for alignment capabilities
   gives a wrong result for cpu32. BTW: alignment capabilities depend not
   only on the CPU. It is also dependant on bus width and how the memory is
   connected.

IMHO, the correct algorithm would be like this:

1. Align dest in any case, no matter what CPU we have. This will do no harm to
   any CPU, since all CPUs can write fast to long-word addresses.
2. After dest is aligned, check whether src is aligned also. If it is aligned,
   we can use optimized algorithm. If not, fall back to bytewise copy. This
   should have been the response to the error reported in the thread mentioned
   above.
3. Some hardware (like cpu32 with 16bit bus) can do long-word access to word
   addresses without speed penalty. With such hardware, having src on an even
   address is enough to use the optimized algorithm.
   BTW: I think this depends not only on the CPU core, but also on how memory
        is connected. I have included 16bit-alignment into the patch anyway.
        We can drop it if it turns out to be true that dependence on the CPU
        is the wrong thing to do here.

Here is a patch for review. Please comment.

diff -ruw newlib-1.18.0.orig/newlib/libc/machine/m68k/memcpy.S newlib-1.18.0/newlib/libc/machine/m68k/memcpy.S
--- newlib-1.18.0.orig/newlib/libc/machine/m68k/memcpy.S       2009-12-14 21:50:53.000000000 +0100
+++ newlib-1.18.0/newlib/libc/machine/m68k/memcpy.S            2010-02-09 14:39:29.326574293 +0100
@@ -15,10 +15,15 @@
 
 #include "m68kasm.h"
 
-#if defined (__mcoldfire__) || defined (__mcpu32__) || defined (__mc68010__) || defined (__mc68020__) || defined (__mc68030__) || defined (__mc68040__) || defined (__mc68060__)
-# define MISALIGNED_OK 1
+#define WORD_ALIGNMENT 1
+#define LONG_ALIGNMENT 3
+
+#if defined (__mcpu32__)
+# define ALIGN_BITS WORD_ALIGNMENT
+#elif defined (__mcoldfire__) || defined (__mc68010__) || defined (__mc68020__) || defined (__mc68030__) || defined (__mc68040__) || defined (__mc68060__)
+ /* don't need alignment */
 #else
-# define MISALIGNED_OK 0
+# define ALIGN_BITS LONG_ALIGNMENT
 #endif
        
        .text
@@ -35,6 +40,9 @@
  *       - make sure the destination pointer (the write pointer) is long word
  *         aligned. This is the best you can do, because writing to unaligned
  *         addresses can be the most costfull thing you could do.
+ *       - after destination pointer is aligned, we still have to check the
+ *         source pointer. If it is not aligned, we have to fall back to
+ *         bytewise copy.
  *       - Once you have figured that out, we do a little loop unrolling
  *         to further improve speed.
  */
@@ -43,33 +51,37 @@
   move.l 4(sp),a0      | dest ptr
   move.l 8(sp),a1      | src ptr
   move.l 12(sp),d1     | len
+
        cmp.l   #8,d1           | if fewer than 8 bytes to transfer,
        blo     .Lresidue       | do not optimise
 
-#if !MISALIGNED_OK
-    /* Goto .Lresidue if either dest or src is not 4-byte aligned */
-    move.l  a0,d0
-    and.l   #3,d0
-    bne     .Lresidue
-    move.l  a1,d0
-    and.l   #3,d0
-    bne     .Lresidue
-#else /* MISALIGNED_OK */
-      /* align dest */
-      move.l   a0,d0           | copy of dest
-      neg.l    d0
-      and.l    #3,d0           | look for the lower two only
-      beq      2f                | is aligned?
-      sub.l    d0,d1
-      lsr.l    #1,d0           | word align needed?
-      bcc      1f
+      move.l   a0,d0           | copy dest
+
+        /* make sure dest is word aligned */
+        btst    #0,d0        | need word align?
+        beq     1f             | no
+
        move.b  (a1)+,(a0)+
+       subq.l  #1,d1
+       move.l  a0,d0           | dest has changed, copy dest again
+
 1:
-       lsr.l   #1,d0           | long align needed?
-       bcc     2f
-       move.w  (a1)+,(a0)+
+        /* make sure dest is long aligned */
+        btst    #1,d0
+        beq     2f             | OK
+
+        move.b (a1)+,(a0)+
+        move.b (a1)+,(a0)+
+        subq.l #2,d1
 2:
-#endif /* !MISALIGNED_OK */
+
+#if defined (ALIGN_BITS)
+    /* dest is aligned now. But src might still be mis-aligned, so check
+           for that */
+           move.l   a1,d0
+           and.l    #ALIGN_BITS,d0
+           bne      .Lresidue
+#endif
 
        /* long word transfers */
        move.l  d1,d0