svn commit: r194110 - head/sys/i386/include

[Christoph Mallon]

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