Re: [PATCH] size reduction of Cell/SPU variable argument part

[Patrick Mansfield <patmans at us dot ibm dot com>]

On Wed, Aug 15, 2007 at 08:26:29PM +0900, Hidetaka Takano wrote:
> To: Jeff Johnston
> Cc: newlib ML members
> 
> I added the "new BSD lisence" to the new files (*.S) of my patch.
> But I did not add it to the other files because of simple and tiny changes.
> 
> Now is this acceptable?

Patch is corrupted, it has some extra newlines. The previous version never
showed up on the newlib list.

Sorry I did not comment on your last internal posting of the patch.

>  lib_a_CCASFLAGS = $(AM_CCASFLAGS)
>  lib_a_CFLAGS = $(AM_CFLAGS)
> @@ -341,6 +335,96 @@
>  lib_a-setjmp.obj: setjmp.S
>  	$(CCAS) $(lib_a_CCASFLAGS) $(CCASFLAGS) -c -o lib_a-setjmp.obj `if test -f 'setjmp.S'; then $(CYGPATH_W) 'setjmp.S'; else
> $(CYGPATH_W) '$(srcdir)/setjmp.S'; fi`
> 

You can see the extra newline above.

> Index: newlib/libc/machine/spu/c99ppe.h
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/machine/spu/c99ppe.h,v
> retrieving revision 1.4
> diff -u -r1.4 c99ppe.h
> --- newlib/libc/machine/spu/c99ppe.h	4 Apr 2007 21:03:13 -0000	1.4
> +++ newlib/libc/machine/spu/c99ppe.h	15 Aug 2007 10:42:23 -0000
> @@ -30,11 +30,26 @@
>  Author: Joel Schopp <jschopp@austin.ibm.com>
>  */
> 
> +#define SPE_C99_SIGNALCODE 0x2100
> +
> +#ifdef __ASSEMBLER__
> +
> +#define STACK_REGS	72	/* Number of registers preserved in stack
> +				  in case of variable argument API. */
> +
> +#define SPE_C99_VFPRINTF	0x23
> +#define SPE_C99_VFSCANF	 	0x24
> +#define SPE_C99_VPRINTF	 	0x25
> +#define SPE_C99_VSCANF	 	0x26
> +#define SPE_C99_VSNPRINTF	0x27
> +#define SPE_C99_VSPRINTF	0x28
> +#define SPE_C99_VSSCANF	 	0x29
> +
> +#else /* __ASSEMBLER__ */

Can you change all the enum SPE_C99_nnnn to defines, so we don't have some
values in the include file twice? And then use #ifndef __ASSEMBLER__
around other hunks. I'd rather they were decimal values like we have in
libgloss/spu/jsre.h.

> Index: newlib/libc/machine/spu/sprintf.S
> ===================================================================
> RCS file: newlib/libc/machine/spu/sprintf.S
> diff -N newlib/libc/machine/spu/sprintf.S
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ newlib/libc/machine/spu/sprintf.S	1 Jan 1970 00:00:00 -0000
> @@ -0,0 +1,56 @@
> +/*
> +  Copyright (c) 2007, Toshiba Corporation
> +
> +  All rights reserved.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions are met:
> +
> +    * Redistributions of source code must retain the above copyright notice,
> +  this list of conditions and the following disclaimer.
> +    * Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in the
> +  documentation and/or other materials provided with the distribution.
> +    * Neither the names of Toshiba nor the names of its
> +  contributors may be used to endorse or promote products derived from this
> +  software without specific prior written permission.
> +
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> +  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> +  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> +  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> +  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> +  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> +  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> +  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> +  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> +  POSSIBILITY OF SUCH DAMAGE.
> +  */
> +
> +#include "c99ppe.h"
> +
> +#define SIGCODE SPE_C99_SIGNALCODE
> +#define NAME    sprintf
> +#define PARMS   2
> +#define OPCODE  SPE_C99_VSPRINTF

Just use the string for SIGCODE, NAME, and OPCODE rather than have a
define. PARMS is OK, or remove it and add a comment.

> +
> +#define parms	$2
> +
> +	.text
> +	.align	4
> +	GLOBL	NAME
> +NAME:

i.e. use sprintf for NAME in the above.

> +	stqd	$0, 16($sp)		/* save caller address */
> +	il	parms, PARMS
> +	brsl	$0, __stack_reg_va	/* save register to the stack frame */
> +
> +	il	$3, SIGCODE		/* signal code */
> +	il	$4, OPCODE		/* op code */
> +	ai	$5, $sp, 16*2		/* data ($3 save address) */
> +	brsl	$0, __send_to_ppe
> +
> +	il	$2, 16*(STACK_REGS+2+2)
> +	a	$sp, $sp, $2
> +	lqd	$0, 16($sp)		/* load caller address */
> +	bi      $0			/* return to caller */
> Index: newlib/libc/machine/spu/fprintf.S
> ===================================================================
> RCS file: newlib/libc/machine/spu/fprintf.S
> diff -N newlib/libc/machine/spu/fprintf.S
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ newlib/libc/machine/spu/fprintf.S	1 Jan 1970 00:00:00 -0000
> @@ -0,0 +1,63 @@
> +/*
> +  Copyright (c) 2007, Toshiba Corporation
> +
> +  All rights reserved.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions are met:
> +
> +    * Redistributions of source code must retain the above copyright notice,
> +  this list of conditions and the following disclaimer.
> +    * Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in the
> +  documentation and/or other materials provided with the distribution.
> +    * Neither the names of Toshiba nor the names of its
> +  contributors may be used to endorse or promote products derived from this
> +  software without specific prior written permission.
> +
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> +  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> +  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> +  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> +  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> +  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> +  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> +  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> +  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> +  POSSIBILITY OF SUCH DAMAGE.
> +  */
> +
> +#include "c99ppe.h"
> +
> +#define SIGCODE SPE_C99_SIGNALCODE
> +#define NAME    fprintf
> +#define PARMS   2
> +#define OPCODE  SPE_C99_VFPRINTF
> +#define FP      3
> +
> +#define parms	$2
> +
> +	.text
> +	.align	4
> +	GLOBL	NAME
> +NAME:
> +	stqd	$0, 16($sp)		/* save caller address */
> +	il	parms, PARMS
> +	brsl	$0, __stack_reg_va	/* save register to the stack frame */
> +
> +	brsl	$0, __check_init
> +	lqd	$3, 16*(FP-1)($sp)	/* $3 <- saved FP on the stack frame */
> +	lqd	$2, 0($3)		/* FP = fp->_fp */
> +	rotqby	$2, $2, $3
> +	stqd	$2, 16*(FP-1)($sp)	/* replace FP on the stack frame */

Why is FP a define value? It is always 3, and the file pointer is always
the first argument to fprintf and fscanf.

> +
> +	il	$3, SIGCODE		/* signal code */
> +	il	$4, OPCODE		/* op code */
> +	ai	$5, $sp, 16*2		/* data ($3 save address) */
> +	brsl	$0, __send_to_ppe
> +
> +	il	$2, 16*(STACK_REGS+2+2)
> +	a	$sp, $sp, $2
> +	lqd	$0, 16($sp)		/* load caller address */
> +	bi      $0			/* return to caller */
> Index: newlib/libc/machine/spu/siprintf.S

IMHO, include mk_syscall and the syscall.def even though they are not
automatically run, so the assembly code can be easily fixed and
regenerated (like if we removed the check_init code).

-- Patrick Mansfield