From owner-svn-src-projects@FreeBSD.ORG Fri Aug 2 11:31:29 2013 Return-Path: Delivered-To: svn-src-projects@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 03060CFE; Fri, 2 Aug 2013 11:31:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 810CD2283; Fri, 2 Aug 2013 11:31:24 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id D37441A2E12; Fri, 2 Aug 2013 21:31:14 +1000 (EST) Date: Fri, 2 Aug 2013 21:31:13 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jung-uk Kim Subject: Re: svn commit: r253872 - in projects/atomic64/sys: amd64/include i386/include In-Reply-To: <201308012320.r71NKWlN083697@svn.freebsd.org> Message-ID: <20130802201545.Y1195@besplex.bde.org> References: <201308012320.r71NKWlN083697@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=Yos2GeoX c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=GPtdIDiJuvUA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=319NO13Q6JwA:10 a=tgUtnBsCqbOwcrNU3w8A:9 a=CjuIK1q_8ugA:10 Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Aug 2013 11:31:29 -0000 On Thu, 1 Aug 2013, Jung-uk Kim wrote: > Log: > Allow any register to be used for the return value. I thought I fixed this in 2006. Actually, I only fixed this for the CPU_DISABLE_CMPXCHG case, which shouldn't exist and only exists on i386. > Modified: projects/atomic64/sys/amd64/include/atomic.h > ============================================================================== > --- projects/atomic64/sys/amd64/include/atomic.h Thu Aug 1 23:02:36 2013 (r253871) > +++ projects/atomic64/sys/amd64/include/atomic.h Thu Aug 1 23:20:31 2013 (r253872) > @@ -138,14 +138,13 @@ atomic_cmpset_int(volatile u_int *dst, u > > __asm __volatile( > " " MPLOCKED " " > - " cmpxchgl %2,%1 ; " > + " cmpxchgl %3, %1 ; " > " sete %0 ; " > - "1: " > "# atomic_cmpset_int" > - : "=a" (res), /* 0 */ > - "=m" (*dst) /* 1 */ > - : "r" (src), /* 2 */ > - "a" (expect), /* 3 */ > + : "=r" (res), /* 0 */ This should be "=q". On amd64, most registers are QI registers so "=r" might work, but it is easier to use the correct constraint. > + "=m" (*dst), /* 1 */ > + "+a" (expect) /* 2 */ The patch is hard to read due to rotating the registers to indicate that 'a' is input-output. I now seem to remember that the hard-coded 'a' register and the difference for the CPU_DISABLE_CMPXCHG cases were intentional. In the CPU_DISABLE_CMPXCHG case, the comparison doesn't clobber any register, so we should use "=q" to let the compiler do the allocation and not copy the "=a" from the !CPU_DISABLE_CMPXCHG case. In the !CPU_DISABLE_CMPXCHG case, we get simpler asm and simplify the task of the register allocator by forcing the useful result into %alm overwriting the clobbered input. Otherwise, the compiler has to notice that %al is dead and reuse it. Not reusing it would require an extra register. > + : "r" (src), /* 3 */ > "m" (*dst) /* 4 */ > : "memory", "cc"); > > ... > Modified: projects/atomic64/sys/i386/include/atomic.h > ============================================================================== > --- projects/atomic64/sys/i386/include/atomic.h Thu Aug 1 23:02:36 2013 (r253871) > +++ projects/atomic64/sys/i386/include/atomic.h Thu Aug 1 23:20:31 2013 (r253872) > @@ -246,14 +246,13 @@ atomic_cmpset_int(volatile u_int *dst, u > > __asm __volatile( > " " MPLOCKED " " > - " cmpxchgl %2,%1 ; " > - " sete %0 ; " > - "1: " > + " cmpxchgl %3, %1 ; " > + " sete %0 ; " > "# atomic_cmpset_int" > - : "=a" (res), /* 0 */ > - "=m" (*dst) /* 1 */ > - : "r" (src), /* 2 */ > - "a" (expect), /* 3 */ > + : "=r" (res), /* 0 */ This must be "=q". On i386, some registers are not QI registers. "=r" gives a syntax error in the generated asm if a non-QI register is chosen. > + "=m" (*dst), /* 1 */ > + "+a" (expect) /* 2 */ > + : "r" (src), /* 3 */ > "m" (*dst) /* 4 */ > : "memory", "cc"); > I checked one of my old versions for fixing this. It is too large and buggy to show here. It uses the 'q' constraint. The only other good thing in it is is a reminder that the '+a' constraint is a style bug when used as above. The following should be used: u_int junk; ... "=a" (junk) ... "a" (expect) This takes another operand, but makes it clearer that the ouput is unused and has nothing to do with 'expect'. Your version works by abusing 'expect' as a scratch variable to discard the ouput in. 'expect' is a function parameter that isn't used again, so cloberring it is harmless. It is just confusing for both humans and compilers. Compilers should see that both 'junk' and 'expect' are dead, so they can reuse the register previously holding them (the 'a' register here must be used to hold 'expect' as input and then should be used to hold 'res' as output). However, with -g, compilers should generate larger code to keep the clobbered 'expect' and the initialized 'junk' alive for debugging, and normally this is more useful for arg variables, but here it is less useful. Bruce