Fix wrong register usage for integer binops

When doing of integers where we use a three argument variant of masm
(lhs/rhs/target), we need three general purpose registers. If the target temp
of the binop is a register, we use that as a target, otherwise fall back to
ReturnValueRegister (scratch). In that case we don't need to move from RVR to
target register. Additionally we need to load lhs and rhs into registers, and
for the lhs we use the target register and for the rhs the scratch register.

So we start by loading the lhs into the target register and the rhs into
the scratch register. However if the rhs is already assigned to a register
and that register happens to be the target register, then the earlier load
of the lhs into the target register overwrote our rhs!

This is fixed by being more careful in the choice of the target temp's assigned
register as "scratch" register for the lhs, i.e. don't use it if the target
temp is also assigned to the same register as the rhs.

Task-number: QTBUG-38097
Change-Id: I2ffec55cb98818fa9ebb5a76a32b6dca72175893
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

1 files changed, 8 insertions, 23 deletions

diff --git a/src/qml/jit/qv4binop.cpp b/src/qml/jit/qv4binop.cpp
index a505221e87..74024752cf 100644
--- a/src/qml/jit/qv4binop.cpp
+++ b/src/qml/jit/qv4binop.cpp
@@ -240,11 +240,14 @@ void Binop::doubleBinop(IR::Expr *lhs, IR::Expr *rhs, IR::Temp *target)
 bool Binop::int32Binop(IR::Expr *leftSource, IR::Expr *rightSource, IR::Temp *target)
 {
     Q_ASSERT(leftSource->type == IR::SInt32Type);
-    Assembler::RegisterID targetReg;
-    if (target->kind == IR::Temp::PhysicalRegister)
-        targetReg = (Assembler::RegisterID) target->index;
-    else
-        targetReg = Assembler::ReturnValueRegister;
+    Assembler::RegisterID targetReg = Assembler::ReturnValueRegister;
+    if (target->kind == IR::Temp::PhysicalRegister) {
+        // We try to load leftSource into the target's register, but we can't do that if
+        // the target register is the same as rightSource.
+        IR::Temp *rhs = rightSource->asTemp();
+        if (!rhs || rhs->kind != IR::Temp::PhysicalRegister || rhs->index != target->index)
+            targetReg = (Assembler::RegisterID) target->index;
+    }
 
     switch (op) {
     case IR::OpBitAnd: {
@@ -338,12 +341,6 @@ bool Binop::int32Binop(IR::Expr *leftSource, IR::Expr *rightSource, IR::Temp *ta
     case IR::OpAdd: {
         Q_ASSERT(rightSource->type == IR::SInt32Type);
 
-        Assembler::RegisterID targetReg;
-        if (target->kind == IR::Temp::PhysicalRegister)
-            targetReg = (Assembler::RegisterID) target->index;
-        else
-            targetReg = Assembler::ReturnValueRegister;
-
         as->add32(as->toInt32Register(leftSource, targetReg),
                    as->toInt32Register(rightSource, Assembler::ScratchRegister),
                    targetReg);
@@ -363,12 +360,6 @@ bool Binop::int32Binop(IR::Expr *leftSource, IR::Expr *rightSource, IR::Temp *ta
             return true;
         }
 
-        Assembler::RegisterID targetReg;
-        if (target->kind == IR::Temp::PhysicalRegister)
-            targetReg = (Assembler::RegisterID) target->index;
-        else
-            targetReg = Assembler::ReturnValueRegister;
-
         as->move(as->toInt32Register(leftSource, targetReg), targetReg);
         as->sub32(as->toInt32Register(rightSource, Assembler::ScratchRegister), targetReg);
         as->storeInt32(targetReg, target);
@@ -376,12 +367,6 @@ bool Binop::int32Binop(IR::Expr *leftSource, IR::Expr *rightSource, IR::Temp *ta
     case IR::OpMul: {
         Q_ASSERT(rightSource->type == IR::SInt32Type);
 
-        Assembler::RegisterID targetReg;
-        if (target->kind == IR::Temp::PhysicalRegister)
-            targetReg = (Assembler::RegisterID) target->index;
-        else
-            targetReg = Assembler::ReturnValueRegister;
-
         as->mul32(as->toInt32Register(leftSource, targetReg),
                    as->toInt32Register(rightSource, Assembler::ScratchRegister),
                    targetReg);