From owner-freebsd-current Thu Dec 13 3:31:36 2001 Delivered-To: freebsd-current@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id 4FE8B37B416; Thu, 13 Dec 2001 03:31:28 -0800 (PST) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id WAA18813; Thu, 13 Dec 2001 22:31:25 +1100 Date: Thu, 13 Dec 2001 22:32:20 +1100 (EST) From: Bruce Evans X-X-Sender: To: John Baldwin Cc: Subject: Re: Patch Review: i386 asm cleanups in the kernel In-Reply-To: Message-ID: <20011213215212.O277-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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