Re: [PATCH] x86/time: fix scale_delta() inline assembly

[Mats Petersson <mats.petersson@citrix.com>]

On 26/11/12 16:31, Ian Campbell wrote:
> On Mon, 2012-11-26 at 15:23 +0000, Jan Beulich wrote:
>> The way it was coded, it clobbered %rdx without telling the compiler.
>> This generally didn't cause any problems except when there are two back
>> to back invocations (as in plt_overflow()), as in that case the
>> compiler may validly assume that it can re-use for the second instance
>> the value loaded into %rdx before the first one.
>>
>> Once at it, also properly relax the second operand of "mul" (there's no
>> need for it to be in %rdx, or a register at all), and switch away from
>> using explicit register names in the instruction operands.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -127,8 +127,9 @@ static inline u64 scale_delta(u64 delta,
>>           delta <<= scale->shift;
>>   
>>       asm (
>> -        "mul %%rdx ; shrd $32,%%rdx,%%rax"
>> -        : "=a" (product) : "0" (delta), "d" ((u64)scale->mul_frac) );
>> +        "mul %2 ; shrd $32,%1,%0"
>> +        : "=a" (product), "=d" (delta)
>> +        : "rm" (delta), "0" ((u64)scale->mul_frac) );
> OOI is there a reason to favour an output constraint overwriting delta,
> which is then thrown away, over a straightforward clobber of rdx?
>
> It took me ages to figure out that you had swapped which of the two
> inputs to the multiply was in rax (I was very confused until I spotted
> that!).
>
> It's also a bit confusing to use %1 and %0 with the shrd since you
> aren't actually using the things specified by the constraints but rather
> the outputs of the mul which happen to (now) be in those registers, I'd
> argue that the previous use of the explicit names was more correctly
> describing what was going on. As it is I was wondering how
>          shrd $32,delta,product
> could possibly make sense.
>
> Since delta is the first argument to the containing function would it be
> helpful to put that in rax, where it already is due to the calling
> convention? I suppose any difference is overwhelmed by the multiply
> anyway.

Probably makes no difference which register is used for the input, as 
the compiler is hopefully inlining the entire function, and thus the 
register which is used as "first argument" is whatever that value 
happens to be in when the compiler gets there. This is one of many 
reasons that makes inlining more efficient - less register pressure. 
Hard-coding register usage in the assembler is a good way to "make life 
difficult for the compiler" (as it now has to make sure the value is in 
that register).

And multiply on modern processors is pretty quick - 8 clocks latency in 
the case of MUL of 64 x 64 bit number on AMD processors - I'm pretty 
sure Intel figures are similar, but not sure where to find that 
information quickly, so couldn't be bothered to look it up. The shrd 
instruction adds another 6 clocks of latency to the whole thing (again 
AMD number).

I'm not sure how many cycles you'd change the code by if the registers 
are hard-coded or compiler can choose... But not hardcoding it would be 
the better choice.

--
Mats
>
> Ian.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>