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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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.

Ian.


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