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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-11-26 at 16:43 +0000, Jan Beulich wrote:
> >>> 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.

Ah, of course.

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

I think the important difference here is that the mul generates its
result specifically in rdx:rax rather than user specified registers.
Since the output is not specified via the constraints IMHO using the
constraints when subsequently consuming those outputs is confusing.

I'd agree with you if mul was something like "mul %2,%0,%1" (where %2*%
rax =>%0:%1) then "shrd $32,%0,%1" would be perfectly fine.

Anyway I guess we'll have to agree to disagree. In any case your opinion
trumps mine on this code ;-)

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

Oops, yes.

> This being an inline function, there's no calling convention anyway.

Right.

Ian.


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