[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel




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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.