Date: Fri, 2 Aug 2013 21:31:13 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jung-uk Kim <jkim@FreeBSD.org> Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r253872 - in projects/atomic64/sys: amd64/include i386/include Message-ID: <20130802201545.Y1195@besplex.bde.org> In-Reply-To: <201308012320.r71NKWlN083697@svn.freebsd.org> References: <201308012320.r71NKWlN083697@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130802201545.Y1195>