Date: Sat, 13 Jun 2009 21:27:12 +0200 From: Christoph Mallon <christoph.mallon@gmx.de> To: Alan Cox <alc@cs.rice.edu> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Ed Schouten <ed@FreeBSD.org> Subject: Re: svn commit: r194110 - head/sys/i386/include Message-ID: <4A33FD90.2080106@gmx.de> In-Reply-To: <4A33EB91.2000605@cs.rice.edu> References: <200906131356.n5DDu6bT015673@svn.freebsd.org> <4A33EB91.2000605@cs.rice.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
Alan Cox schrieb: > Ed Schouten wrote: >> Author: ed >> Date: Sat Jun 13 13:56:06 2009 >> New Revision: 194110 >> URL: http://svn.freebsd.org/changeset/base/194110 >> >> Log: >> Simplify the inline assembler (and correct potential error) of >> pte_load_store(). >> Submitted by: Christoph Mallon >> >> Modified: >> head/sys/i386/include/pmap.h >> >> Modified: head/sys/i386/include/pmap.h >> ============================================================================== >> >> --- head/sys/i386/include/pmap.h Sat Jun 13 13:54:03 2009 (r194109) >> +++ head/sys/i386/include/pmap.h Sat Jun 13 13:56:06 2009 (r194110) >> @@ -362,15 +362,8 @@ pte_load(pt_entry_t *ptep) >> static __inline pt_entry_t >> pte_load_store(pt_entry_t *ptep, pt_entry_t pte) >> { >> - pt_entry_t r; >> - >> - __asm __volatile( >> - "xchgl %0,%1" >> - : "=m" (*ptep), >> - "=r" (r) >> - : "1" (pte), >> - "m" (*ptep)); >> - return (r); >> + __asm volatile("xchgl %0, %1" : "+m" (*ptep), "+r" (pte)); >> + return (pte); >> } >> >> #define pte_load_clear(pte) atomic_readandclear_int(pte) >> > I'm afraid that this change violates the rules, specifically, "+" cannot > be combined with "m": > > File: gcc.info, Node: Extended Asm, Next: Constraints, Prev: Inline, > Up: C Extensions > > 5.35 Assembler Instructions with C Expression Operands > ====================================================== > > ... Extended asm supports input-output or read-write > operands. Use the constraint character `+' to indicate such an operand > and list it with the output operands. You should only use read-write > operands when the constraints for the operand (or the operand in which > only some of the bits are to be changed) allow a register. It depends which part and version of the documentation you read. Take this slightly different version of the same passage: --- [...] Extended asm supports input-output or read-write operands. Use the constraint character + to indicate such an operand and list it with the output operands. When the constraints for the read-write operand (or the operand in which only some of the bits are to be changed) allows a register, you may, as an alternative, logically split its function into two separate operands, one input operand and one write-only output operand. The connection between them is expressed by constraints which say they need to be in the same location when the instruction executes. You can use the same C expression for both operands, or different expressions. For example, here we write the (fictitious) combine instruction with bar as its read-only source operand and foo as its read-write destination: asm ("combine %2,%0" : "=r" (foo) : "0" (foo), "g" (bar)); The constraint "0" for operand 1 says that it must occupy the same location as operand 0. A number in constraint is allowed only in an input operand and it must refer to an output operand. Only a number in the constraint can guarantee that one operand will be in the same place as another. The mere fact that foo is the value of both operands is not enough to guarantee that they will be in the same place in the generated assembler code. The following would not work reliably: asm ("combine %2,%0" : "=r" (foo) : "r" (foo), "g" (bar)); --- If you read this carefully, the conclusion is that the previous "=m" and "m" was invalid, because there was no direction connection between the two. Further, the explanation of '+' itself (5.36.3) does not mention anything about what you quoted from 5.35. Last, GCC itself uses +m in its backend instruction specifications: --- ;; Recall that xchg implicitly sets LOCK#, so adding it again wastes space. (define_insn "sync_lock_test_and_set<mode>" [(set (match_operand:IMODE 0 "register_operand" "=<modeconstraint>") (unspec_volatile:IMODE [(match_operand:IMODE 1 "memory_operand" "+m")] UNSPECV_XCHG)) (set (match_dup 1) (match_operand:IMODE 2 "register_operand" "0"))] "" "xchg{<modesuffix>}\t{%1, %0|%0, %1}") --- The above implementation of the __sync_lock_test_and_set() builtin (section 5.45) performs the same operation as pte_load_store() and it uses the +m constraint for its memory operand. I rather trust the code itself than the documentation. Christoph
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4A33FD90.2080106>