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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.11.12 at 17:31, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxx>
>> 
>> --- 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?

Yes - with a clobber of %rdx the compiler can't pass "delta" in that
register.

> 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!).

Sorry. The resulting code was better (and less different from the
prior version) with the operands swapped.

> 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.

That's how things are with multi-insn asm-s (and even with single-
insn ones when an output different from an input get tied together
through using the same register). I continue to think that using the
numbered forms here is better.

> 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.

The first argument of a "real" function would be in %rdi. This being
an inline function, there's no calling convention anyway.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel