Date: Fri, 14 Dec 2001 09:09:27 +0800 From: David Xu <davidx@viasoft.com.cn> To: John Baldwin <jhb@FreeBSD.ORG> Cc: Bruce Evans <bde@zeta.org.au>, current@FreeBSD.ORG Subject: Re: Patch Review: i386 asm cleanups in the kernel Message-ID: <3C195147.7000605@viasoft.com.cn> References: <XFMail.011213112756.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3C195147.7000605>