Date: Thu, 13 Dec 2001 22:32:20 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: John Baldwin <jhb@FreeBSD.org> Cc: <current@FreeBSD.org> Subject: Re: Patch Review: i386 asm cleanups in the kernel Message-ID: <20011213215212.O277-100000@gamplex.bde.org> In-Reply-To: <XFMail.011211150216.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 11 Dec 2001, John Baldwin wrote:
> Ok, I've axed all the "cc" clobbers from the patch now. Any objections to it
> now? It's still at ~jhb/patches/i386_asm.patch.
Review of what I can see: the URL still appears to be well formed ;-). But
it didn't seem to work, so I scp'ed the file.
> Index: i386/i386/identcpu.c
> ===================================================================
> RCS file: /usr/cvs/src/sys/i386/i386/identcpu.c,v
> retrieving revision 1.96
> diff -u -r1.96 identcpu.c
> --- i386/i386/identcpu.c 30 Nov 2001 11:57:23 -0000 1.96
> +++ i386/i386/identcpu.c 6 Dec 2001 07:58:25 -0000
> @@ -115,10 +115,11 @@
> static void
> do_cpuid(u_int ax, u_int *p)
> {
> +
> + p[0] = ax;
> __asm __volatile(
> "cpuid"
> - : "=a" (p[0]), "=b" (p[1]), "=c" (p[2]), "=d" (p[3])
> - : "0" (ax)
> + : "+a" (p[0]), "=b" (p[1]), "=c" (p[2]), "=d" (p[3])
> );
> }
>
"0" here was bogus. Just replace it by "a".
> Index: i386/i386/math_emulate.c
> ===================================================================
> RCS file: /usr/cvs/src/sys/i386/i386/math_emulate.c,v
> retrieving revision 1.40
> diff -u -r1.40 math_emulate.c
> --- i386/i386/math_emulate.c 28 Nov 2001 01:42:16 -0000 1.40
> +++ i386/i386/math_emulate.c 7 Dec 2001 19:07:16 -0000
> ...
> @@ -771,14 +770,13 @@
> "addl %0,%0 ; adcl %1,%1\n\t" \
> "addl %0,%0 ; adcl %1,%1\n\t" \
> "addl %%ecx,%0 ; adcl %%ebx,%1" \
> - : "=a" (low), "=d" (high) \
> - : "0" (low), "1" (high) \
> - : "cx", "bx")
> + : "+a" (low), "+d" (high) \
> + : : "ecx", "ebx")
Weren't the register names in the clobber list the normal ones?
> @@ -938,7 +935,7 @@
> "movl 4(%0),%%eax ; adcl %%eax,4(%0)\n\t"
> "movl 8(%0),%%eax ; adcl %%eax,8(%0)\n\t"
> "movl 12(%0),%%eax ; adcl %%eax,12(%0)"
> - ::"r" (c):"ax");
> + : :"r" (c):"eax", "memory");
> }
>
> static void
This should use output operands c[0]..c[3], not a general "memory" clobber.
> [... more suboptimal "memory" clobbers]
I wouldn't trust all the little changes to math_emulate.c without runtime
testing. It won't be tested by using it in normal operation.
> Index: i386/include/bus_pc98.h
> ===================================================================
> RCS file: /usr/cvs/src/sys/i386/include/bus_pc98.h,v
> retrieving revision 1.19
> diff -u -r1.19 bus_pc98.h
> --- i386/include/bus_pc98.h 7 Oct 2001 10:04:18 -0000 1.19
> +++ i386/include/bus_pc98.h 7 Dec 2001 19:10:37 -0000
> @@ -203,11 +203,10 @@
> \
> __asm __volatile("call *%2" \
> :"=a" (result), \
> - "=d" (offset) \
> + "+d" (offset) \
> :"o" (bsh->bsh_bam.bs_read_##BWN), \
> - "b" (bsh), \
> - "1" (offset) \
> - ); \
> + "b" (bsh) \
> + :"ecx","memory"); \
> \
> return result; \
> }
It's surprising that the missing "ecx" in lots of clobber lists in this
file didn't cause problems.
> Index: i386/include/cpufunc.h
> ===================================================================
> RCS file: /usr/cvs/src/sys/i386/include/cpufunc.h,v
> retrieving revision 1.105
> diff -u -r1.105 cpufunc.h
> --- i386/include/cpufunc.h 28 Jun 2001 02:08:13 -0000 1.105
> +++ i386/include/cpufunc.h 7 Dec 2001 19:11:47 -0000
> @@ -66,18 +66,18 @@
> static __inline u_int
> bsfl(u_int mask)
> {
> - u_int result;
> + u_int result = mask;
Style bug: initialization in declaration.
>
> - __asm __volatile("bsfl %0,%0" : "=r" (result) : "0" (mask));
> + __asm __volatile("bsfl %0,%0" : "+r" (result));
> return (result);
> }
"0" here was bogus, except we abuse the same operand for input and output.
I checked that gcc does the right thing (reuse the input register for
output if the input operand becomes dead) with a non-hand-optimized version:
__asm __volatile("bsfl %1,%0" : "=r" (result) : "r" (mask));
...
main() { return bsfl(23); }
"rm" (mask) works too.
>
> static __inline u_int
> bsrl(u_int mask)
> {
> - u_int result;
> + u_int result = mask;
>
> - __asm __volatile("bsrl %0,%0" : "=r" (result) : "0" (mask));
> + __asm __volatile("bsrl %0,%0" : "+r" (result));
> return (result);
> }
Similarly.
The other changes seem to be OK. I checked about 1/4 of them.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20011213215212.O277-100000>