Re: [Xen-devel] [PATCH v2] x86/atomic: Improvements and simplifications to assembly constraints

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.19 at 21:19, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/03/2019 14:58, Jan Beulich wrote:
>>>>> On 18.03.19 at 15:11, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 18/03/2019 13:20, Jan Beulich wrote:
>>>>  >>> On 18.03.19 at 12:27, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> * Some of the single-byte versions specify "=q" as the output.  This is a
>>>>>    remnent of the 32bit build and can be relaxed to "=r" in 64bit builds.
>>>> I have to admit that I don't understand the "relaxed" part of this:
>>>> "q" and "r" represent the exact same set of registers on 64-bit.
>>>> Unless the conversion allows further code folding, I think it wouldn't
>>>> be a bad idea to retain the distinction, just for cases like code
>>>> eventually getting shared via something like lib/x86/.
>>> The change from =q to =r is specifically to allow the folding to +r
>> Hmm, I still don't understand. "+q" is as valid as "+r". And "q" does
>> not make any size implications, i.e. registers in that group aren't
>> implicitly byte ones (albeit this aspect doesn't even matter here).
> 
> I'm at a complete loss to understand what you are objecting to here.

The main point of my comment is your use of the word "relax" when
there's nothing you really relax (afaics - below you confirm yourself
that the two are synonyms on 64-bit).

> "q" and "r" mean different things on 32bit, and "+q" would be wrong to
> use there.

Why would "+q" be wrong? You want "q" (to constrain the register
set to those registers which are byte accessible) and you also want
"+" (as it's an in/out, and the folding from "=q" plus "0" is as valid as
the folding from "=r" plus "0" to "+r").

> "q" and "r" are synonymous on 64bit, but "r" is still the one to use
> because that is the more common constraint, and therefore the more
> familiar for people reading the code.

I understand this part of your argument; I don't see you responding
at all to the potential code re-usability point I've been making. Yet
there's weighing to be done between the two.

>>>>> @@ -40,28 +37,24 @@ static always_inline unsigned long __xchg(
>>>>>      switch ( size )
>>>>>      {
>>>>>      case 1:
>>>>> -        asm volatile ( "xchgb %b0,%1"
>>>>> -                       : "=q" (x)
>>>>> -                       : "m" (*__xg(ptr)), "0" (x)
>>>>> -                       : "memory" );
>>>>> +        asm volatile ( "xchg %b[x], %[ptr]"
>>>>> +                       : [x] "+r" (x), [ptr] "+m" (*(uint8_t *)ptr)
>>>>> +                       :: "memory" );
>>>>>          break;
>>>>>      case 2:
>>>>> -        asm volatile ( "xchgw %w0,%1"
>>>>> -                       : "=r" (x)
>>>>> -                       : "m" (*__xg(ptr)), "0" (x)
>>>>> -                       : "memory" );
>>>>> +        asm volatile ( "xchg %w[x], %[ptr]"
>>>>> +                       : [x] "+r" (x), [ptr] "+m" (*(uint16_t *)ptr)
>>>>> +                       :: "memory" );
>>>>>          break;
>>>>>      case 4:
>>>>> -        asm volatile ( "xchgl %k0,%1"
>>>>> -                       : "=r" (x)
>>>>> -                       : "m" (*__xg(ptr)), "0" (x)
>>>>> -                       : "memory" );
>>>>> +        asm volatile ( "xchg %k[x], %[ptr]"
>>>>> +                       : [x] "+r" (x), [ptr] "+m" (*(uint32_t *)ptr)
>>>>> +                       :: "memory" );
>>>>>          break;
>>>>>      case 8:
>>>>> -        asm volatile ( "xchgq %0,%1"
>>>>> -                       : "=r" (x)
>>>>> -                       : "m" (*__xg(ptr)), "0" (x)
>>>>> -                       : "memory" );
>>>>> +        asm volatile ( "xchg %q[x], %[ptr]"
>>>>> +                       : [x] "+r" (x), [ptr] "+m" (*(uint64_t *)ptr)
>>>>> +                       :: "memory" );
>>>>>          break;
>>>> Is the q modifier really useful to have here (and elsewhere below)?
>>> Yes - it is strictly necessary, because otherwise it gets derived from
>>> the type of (x) which is unsigned long even in the smaller size constructs.
>> What you say explains why you need the b, w, and k modifiers. It
>> doesn't explain the q one, since sizeof(unsigned long) is 8 and
>> hence exactly what would result without the modifier.
> 
> Oh, in which case, "for consistency".

Well, okay, albeit along the lines of the code re-usability point made
before (and mentioned above) personally I'd prefer if we kept the
code in a shape where it could _in principle_ be (easily) re-used for
32-bit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel