diff mbox

[ARM] architecture specific subdirectories & optimised memchr [V3]

Message ID 20110928175913.GA11067@davesworkthinkpad
State Superseded
Headers show

Commit Message

Dr. David Alan Gilbert Sept. 28, 2011, 5:59 p.m. UTC 
Hi,
  As previously discussed, here is a new version of my ARM optimised memchr
and patch that picks up ARM version based on compiler flags.

  This version now never tries to use the first part of the triplet to
determine $machine, it always uses compiler flags to set $machine (otherwise
it falls back to machine=arm).   The directorys are armv7 (rather than armv7-a)
and armv6t2.

  This version also adds the cfi_ markers into the assembler; please
tell me if they make sense!

  I've also done bigendian testing with a qemu usermode emulation
and the string tests all pass happily (usermode emulation of be doesn't
seem particularly happy - especially for nptl stuff, but that's true
without my patch as well).

  (Note as I was about to post this I found a few dead lines of code
in the preconfigure diff ; I've redone the build tests for all ARM combinations
and it still seems happy and checked with objdump it's picking the
right memchr, and checked the BE code got the right version -
I'm running the full little endian test suite again at the moment and
will report on any issues on Monday).

 This is against svn rev r15224

Dave

Comments

Roland McGrath Sept. 28, 2011, 6:18 p.m. UTC | #1 
The usual way to do this sort of thing is by using --with-cpu at configure
time.
Dr. David Alan Gilbert Oct. 3, 2011, 8:48 a.m. UTC | #2 
On 28 September 2011 19:18, Roland McGrath <roland@hack.frob.com> wrote:
> The usual way to do this sort of thing is by using --with-cpu at configure
> time.

See discussion http://cygwin.com/ml/libc-ports/2011-08/msg00005.html

The short story is that --with-cpu sets $submachine not $machine; in this case
we're using $machine to pick up ARM variants that have incompatible
extra instructions.
We could use $submachine to pick further subvariants - e.g. machine=armv7
and submachine=cortex-a9 for stuff which just has differences in optimisation.

My first version used $submachine rather $machine to let you do this
but was persuaded
that it was the wrong way!

Dave
Joseph Myers Oct. 26, 2011, 11:42 p.m. UTC | #3 
On Wed, 28 Sep 2011, Dr. David Alan Gilbert wrote:

> Hi,
>   As previously discussed, here is a new version of my ARM optimised memchr
> and patch that picks up ARM version based on compiler flags.

Given the discussions on the newlib list, is this still the version of the 
memchr code you are proposing or is there a newer one?

> +		case x$archcppflag in
> +		x__ARM_ARCH_7A__)
> +		  machine=armv7
> +		  echo "Found compiler is configured for $machine"
> +		  ;;
> +
> +		x__ARM_ARCH_6T2__)
> +		  machine=armv6t2
> +		  echo "Found compiler is configured for $machine"
> +		  ;;

Let's assume we do detect architecture variants this way.  The main 
problem is that this will be suboptimal on any future architecture 
variants - if the compiler defines __ARM_ARCH_8A__, for example.  There 
are various ways to avoid that issue:

* sysdeps/arm/memcpy.S has a conditional on all the macros for older 
architecture variants not being defined.  That's not a very convenient way 
to do things and it's always possible there could be a new variant of 
ARMv6, say.

* Hopefully in the future the compiler will define a macro directly with a 
version number for the architecture, which will make this easier.

* At present, the best approach for a configure script like this is 
probably to handle __ARM_ARCH_[789]* and __ARM_ARCH_1[0-9]* the same as 
your patch handles __ARM_ARCH_7A__ - assume that all v7 and later 
architectures can use this code.
Dr. David Alan Gilbert Oct. 28, 2011, 4:51 p.m. UTC | #4 
On 27 October 2011 00:42, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Wed, 28 Sep 2011, Dr. David Alan Gilbert wrote:
>
>> Hi,
>>   As previously discussed, here is a new version of my ARM optimised memchr
>> and patch that picks up ARM version based on compiler flags.
>
> Given the discussions on the newlib list, is this still the version of the
> memchr code you are proposing or is there a newer one?

I can update with a new one in a few days; the tweeks as discussed on
that list are minor.

>> +             case x$archcppflag in
>> +             x__ARM_ARCH_7A__)
>> +               machine=armv7
>> +               echo "Found compiler is configured for $machine"
>> +               ;;
>> +
>> +             x__ARM_ARCH_6T2__)
>> +               machine=armv6t2
>> +               echo "Found compiler is configured for $machine"
>> +               ;;
>
> Let's assume we do detect architecture variants this way.  The main
> problem is that this will be suboptimal on any future architecture
> variants - if the compiler defines __ARM_ARCH_8A__, for example.  There
> are various ways to avoid that issue:

It's difficult, especially since the naming convention isn't quite linear;
i.e. we have the A variant on 7, I don't know if that convention will
stay for 8; and since it looks like 8 is going to have 32 and 64bit variants
it looks like a much bigger overhaul of the config around that area
will happen.

> * sysdeps/arm/memcpy.S has a conditional on all the macros for older
> architecture variants not being defined.  That's not a very convenient way
> to do things and it's always possible there could be a new variant of
> ARMv6, say.
>
> * Hopefully in the future the compiler will define a macro directly with a
> version number for the architecture, which will make this easier.

Yes, that would be nice.

> * At present, the best approach for a configure script like this is
> probably to handle __ARM_ARCH_[789]* and __ARM_ARCH_1[0-9]* the same as
> your patch handles __ARM_ARCH_7A__ - assume that all v7 and later
> architectures can use this code.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>

Dave
Joseph Myers Oct. 28, 2011, 6:57 p.m. UTC | #5 
On Fri, 28 Oct 2011, David Gilbert wrote:

> It's difficult, especially since the naming convention isn't quite linear;
> i.e. we have the A variant on 7, I don't know if that convention will
> stay for 8; and since it looks like 8 is going to have 32 and 64bit variants
> it looks like a much bigger overhaul of the config around that area
> will happen.

The existing code clearly isn't going to support building for a 64-bit 
ABI, which would need conditionals in installed headers and separate 
sysdeps files as appropriate for non-installed headers and other sources; 
the aim would be to do the right thing for ARMv8, 32-bit ABI.  And it 
seems reasonable to suppose that any 32-bit variant of ARMv8 supporting 
GNU/Linux will support everything from ARMv6T2 (which is what's actually 
required here).
diff mbox

Patch

Index: ports/sysdeps/arm/preconfigure
===================================================================
--- ports/sysdeps/arm/preconfigure	(revision 15384)
+++ ports/sysdeps/arm/preconfigure	(working copy)
@@ -3,6 +3,33 @@ 
 	base_machine=arm
 	case $config_os in
 	linux-gnueabi*)
+		# Lets ask the compiler which ARM family we've got
+		# Unfortunately it doesn't define any flags for implementations
+		# that you might pass to -mcpu or -mtune
+		# Note if you add patterns here you must ensure that
+		# an appropriate directory exists in sysdeps/arm/eabi
+		archcppflag=`echo "" |
+		$CC $CFLAGS $CPPFLAGS -E -dM - |
+		  grep __ARM_ARCH |
+		  sed -e 's/^#define //' -e 's/ .*//'` 
+
+		case x$archcppflag in
+		x__ARM_ARCH_7A__)
+		  machine=armv7
+		  echo "Found compiler is configured for $machine"
+		  ;;
+
+		x__ARM_ARCH_6T2__)
+		  machine=armv6t2
+		  echo "Found compiler is configured for $machine"
+		  ;;
+
+		*)
+		  machine=arm
+		  echo 2>&1 "arm/preconfigure: Did not find ARM architecture type; using default"
+		  ;;
+		esac
+
 		machine=arm/eabi/$machine
 		if [ "${CFLAGS+set}" != "set" ]; then
 		  CFLAGS="-g -O2"
Index: ports/sysdeps/arm/eabi/armv6t2/memchr.S
===================================================================
--- ports/sysdeps/arm/eabi/armv6t2/memchr.S	(revision 0)
+++ ports/sysdeps/arm/eabi/armv6t2/memchr.S	(revision 0)
@@ -0,0 +1,161 @@ 
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Code contributed by Dave Gilbert <david.gilbert@linaro.org>
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <sysdep.h>
+
+@ This memchr routine is optimised on a Cortex-A9 and should work on all ARMv7
+@ and ARMv6T2 processors.  It has a fast past for short sizes, and has an
+@ optimised path for large data sets; the worst case is finding the match early
+@ in a large data set.
+@ Note: The use of cbz/cbnz means it's Thumb only
+
+@ 2011-07-15 david.gilbert@linaro.org
+@    Copy from Cortex strings release 21 and change license
+@ http://bazaar.launchpad.net/~linaro-toolchain-dev/cortex-strings/trunk/view/head:/src/linaro-a9/memchr.S
+@    Change function declarations/entry/exit
+
+@ this lets us check a flag in a 00/ff byte easily in either endianness
+#ifdef __ARMEB__
+#define CHARTSTMASK(c) 1<<(31-(c*8))
+#else
+#define CHARTSTMASK(c) 1<<(c*8)
+#endif
+	.syntax unified
+
+	.text
+	.thumb
+
+@ ---------------------------------------------------------------------------
+	.thumb_func
+	.global memchr
+	.type memchr,%function
+ENTRY(memchr)
+	@ r0 = start of memory to scan
+	@ r1 = character to look for
+	@ r2 = length
+	@ returns r0 = pointer to character or NULL if not found
+	and	r1,r1,#0xff	@ Don't think we can trust the caller to actually pass a char
+
+	cmp	r2,#16		@ If it's short don't bother with anything clever
+	blt	20f
+
+	tst	r0, #7		@ If it's already aligned skip the next bit
+	beq	10f
+
+	@ Work up to an aligned point
+5:
+	ldrb	r3, [r0],#1
+	subs	r2, r2, #1
+	cmp	r3, r1
+	beq	50f		@ If it matches exit found
+	tst	r0, #7
+	cbz	r2, 40f		@ If we run off the end, exit not found
+	bne	5b		@ If not aligned yet then do next byte
+
+10:
+	@ At this point, we are aligned, we know we have at least 8 bytes to work with
+	push	{r4,r5,r6,r7}
+	cfi_adjust_cfa_offset (16)
+	cfi_rel_offset (r4, 0)
+	cfi_rel_offset (r5, 4)
+	cfi_rel_offset (r6, 8)
+	cfi_rel_offset (r7, 12)
+
+	cfi_remember_state
+
+	orr	r1, r1, r1, lsl #8	@ expand the match word across to all bytes
+	orr	r1, r1, r1, lsl #16
+	bic	r4, r2, #7	@ Number of double words to work with
+	mvns	r7, #0		@ all F's
+	movs	r3, #0
+
+15:
+	ldmia	r0!,{r5,r6}
+	subs	r4, r4, #8
+	eor	r5,r5, r1	@ Get it so that r5,r6 have 00's where the bytes match the target
+	eor	r6,r6, r1
+	uadd8	r5, r5, r7	@ Parallel add 0xff - sets the GE bits for anything that wasn't 0
+	sel	r5, r3, r7	@ bytes are 00 for none-00 bytes, or ff for 00 bytes - NOTE INVERSION
+	uadd8	r6, r6, r7	@ Parallel add 0xff - sets the GE bits for anything that wasn't 0
+	sel	r6, r5, r7	@ chained....bytes are 00 for none-00 bytes, or ff for 00 bytes - NOTE INVERSION
+	cbnz	r6, 60f
+	bne	15b		@ (Flags from the subs above) If not run out of bytes then go around again
+
+	pop	{r4,r5,r6,r7}
+	cfi_adjust_cfa_offset (-16)
+	cfi_restore (r4)
+	cfi_restore (r5)
+	cfi_restore (r6)
+	cfi_restore (r7)
+
+	and	r1,r1,#0xff	@ Get r1 back to a single character from the expansion above
+	and	r2,r2,#7	@ Leave the count remaining as the number after the double words have been done
+
+20:
+	cbz	r2, 40f		@ 0 length or hit the end already then not found
+
+21:  @ Post aligned section, or just a short call
+	ldrb	r3,[r0],#1
+	subs	r2,r2,#1
+	eor	r3,r3,r1	@ r3 = 0 if match - doesn't break flags from sub
+	cbz	r3, 50f
+	bne	21b		@ on r2 flags
+
+40:
+	movs	r0,#0		@ not found
+	DO_RET(lr)
+
+50:
+	subs	r0,r0,#1	@ found
+	DO_RET(lr)
+
+60:  @ We're here because the fast path found a hit - now we have to track down exactly which word it was
+     @ r0 points to the start of the double word after the one that was tested
+     @ r5 has the 00/ff pattern for the first word, r6 has the chained value
+	cfi_restore_state
+	cmp	r5, #0
+	itte	eq
+	moveq	r5, r6		@ the end is in the 2nd word
+	subeq	r0,r0,#3	@ Points to 2nd byte of 2nd word
+	subne	r0,r0,#7	@ or 2nd byte of 1st word
+
+	@ r0 currently points to the 3rd byte of the word containing the hit
+	tst	r5, # CHARTSTMASK(0)	@ 1st character
+	bne	61f
+	adds	r0,r0,#1
+	tst	r5, # CHARTSTMASK(1)	@ 2nd character
+	ittt	eq
+	addeq	r0,r0,#1
+	tsteq	r5, # (3<<15)		@ 2nd & 3rd character
+	@ If not the 3rd must be the last one
+	addeq	r0,r0,#1
+
+61:
+	pop	{r4,r5,r6,r7}
+	cfi_adjust_cfa_offset (-16)
+	cfi_restore (r4)
+	cfi_restore (r5)
+	cfi_restore (r6)
+	cfi_restore (r7)
+
+	subs	r0,r0,#1
+	DO_RET(lr)
+
+END(memchr)
+libc_hidden_builtin_def (memchr)
Index: ports/sysdeps/arm/eabi/armv7/Implies
===================================================================
--- ports/sysdeps/arm/eabi/armv7/Implies	(revision 0)
+++ ports/sysdeps/arm/eabi/armv7/Implies	(revision 0)
@@ -0,0 +1,2 @@ 
+# We can do everything that 6T2 can
+arm/eabi/armv6t2