From owner-freebsd-current Thu Dec 13 17:15:16 2001 Delivered-To: freebsd-current@freebsd.org Received: from mail.viasoft.com.cn (unknown [61.153.1.177]) by hub.freebsd.org (Postfix) with ESMTP id 6A09337B416; Thu, 13 Dec 2001 17:14:59 -0800 (PST) Received: from viasoft.com.cn (davidwnt.viasoft.com.cn [192.168.1.239]) by mail.viasoft.com.cn (8.9.3/8.9.3) with ESMTP id JAA07309; Fri, 14 Dec 2001 09:22:35 +0800 Message-ID: <3C195147.7000605@viasoft.com.cn> Date: Fri, 14 Dec 2001 09:09:27 +0800 From: David Xu User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20011019 Netscape6/6.2 X-Accept-Language: en-us MIME-Version: 1.0 To: John Baldwin Cc: Bruce Evans , current@FreeBSD.ORG Subject: Re: Patch Review: i386 asm cleanups in the kernel References: Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit 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 I persist with adding "cc", because it does not hurt anything. -- David Xu John Baldwin wrote: >On 13-Dec-01 Bruce Evans wrote: > >>>--- 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". >> > >So use "a" twice and gcc will get that right? > >>>- : "=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? >> > >I was just being pedantic since we aren't just clobbering the low words of the >registers. > > >>>@@ -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. >> > >Ok. > >>>[... more suboptimal "memory" clobbers] >>> > >I changed the simple ones, but not the harder ones. (gcc doesn't like having >more than 10 operands, even if they are memory or immediates) > >>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. >> > >Ok. I won't commit it until it is tested. > >>>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. >> > >It depends on what the functions do. If they are written in asm, they might be >preserving temporary registers rather than trashing them. I was just changing >the clobbers to assume that only call-safe registers were preserved. > >>>- __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. >> > >Ok, that's simpler then. > >>The other changes seem to be OK. I checked about 1/4 of them. >> > >Ok, well, I've updated the patch at the same location. > >>Bruce >> > To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message