Bug 951527 - SpiderMonkey: Fix codegen for mixed-type MoveGroup cycles. r=jandem

js/src/jit/MoveResolver.cpp

@@ -113,8 +113,8 @@ MoveResolver::resolve()
                     // assert that we do not find two cycles in one move chain
                     // traversal (which would indicate two moves to the same
                     // destination).
-                    pm->setInCycle();
-                    blocking->setInCycle();
+                    pm->setCycleEnd();
+                    blocking->setCycleBegin(pm->type());
                     hasCycles_ = true;
                     pending_.remove(blocking);
                     stack.pushBack(blocking);

js/src/jit/MoveResolver.h

@@ -110,7 +110,8 @@ class MoveOp
   protected:
     MoveOperand from_;
     MoveOperand to_;
-    bool cycle_;
+    bool cycleBegin_;
+    bool cycleEnd_;
 
   public:
     enum Type {
@@ -123,18 +124,30 @@ class MoveOp
   protected:
     Type type_;
 
+    // If cycleBegin_ is true, endCycleType_ is the type of the move at the end
+    // of the cycle. For example, given these moves:
+    //       INT32 move a -> b
+    //     GENERAL move b -> a
+    // the move resolver starts by copying b into a temporary location, so that
+    // the last move can read it. This copy needs to use use type GENERAL.
+    Type endCycleType_;
+
   public:
     MoveOp()
     { }
     MoveOp(const MoveOperand &from, const MoveOperand &to, Type type)
       : from_(from),
         to_(to),
-        cycle_(false),
+        cycleBegin_(false),
+        cycleEnd_(false),
         type_(type)
     { }
 
-    bool inCycle() const {
-        return cycle_;
+    bool isCycleBegin() const {
+        return cycleBegin_;
+    }
+    bool isCycleEnd() const {
+        return cycleEnd_;
     }
     const MoveOperand &from() const {
         return from_;
@@ -145,6 +158,10 @@ class MoveOp
     Type type() const {
         return type_;
     }
+    Type endCycleType() const {
+        JS_ASSERT(isCycleBegin());
+        return endCycleType_;
+    }
 };
 
 class MoveResolver
@@ -161,9 +178,14 @@ class MoveResolver
           : MoveOp(from, to, type)
         { }
 
-        void setInCycle() {
-            JS_ASSERT(!inCycle());
-            cycle_ = true;
+        void setCycleBegin(Type endCycleType) {
+            JS_ASSERT(!isCycleBegin() && !isCycleEnd());
+            cycleBegin_ = true;
+            endCycleType_ = endCycleType;
+        }
+        void setCycleEnd() {
+            JS_ASSERT(!isCycleBegin() && !isCycleEnd());
+            cycleEnd_ = true;
         }
     };
 

js/src/jit/arm/MoveEmitter-arm.cpp

@@ -274,14 +274,16 @@ MoveEmitterARM::emit(const MoveOp &move)
     const MoveOperand &from = move.from();
     const MoveOperand &to = move.to();
 
-    if (move.inCycle()) {
-        if (inCycle_) {
-            completeCycle(from, to, move.type());
-            inCycle_ = false;
-            return;
-        }
+    if (move.isCycleEnd()) {
+        JS_ASSERT(inCycle_);
+        completeCycle(from, to, move.type());
+        inCycle_ = false;
+        return;
+    }
 
-        breakCycle(from, to, move.type());
+    if (move.isCycleBegin()) {
+        JS_ASSERT(!inCycle_);
+        breakCycle(from, to, move.endCycleType());
         inCycle_ = true;
     }
 

js/src/jit/shared/MoveEmitter-x86-shared.cpp

@@ -38,9 +38,8 @@ MoveEmitterX86::characterizeCycle(const MoveResolver &moves, size_t i,
         if (!*allGeneralRegs && !*allFloatRegs)
             return -1;
 
-        // The first and last move of the cycle are marked with inCycle(). Stop
-        // iterating when we see the last one.
-        if (j != i && move.inCycle())
+        // Stop iterating when we see the last one.
+        if (j != i && move.isCycleEnd())
             break;
 
         // Check that this move is actually part of the cycle. This is
@@ -103,14 +102,15 @@ MoveEmitterX86::emit(const MoveResolver &moves)
         const MoveOperand &from = move.from();
         const MoveOperand &to = move.to();
 
-        if (move.inCycle()) {
-            // If this is the end of a cycle for which we're using the stack,
-            // handle the end.
-            if (inCycle_) {
-                completeCycle(to, move.type());
-                inCycle_ = false;
-                continue;
-            }
+        if (move.isCycleEnd()) {
+            JS_ASSERT(inCycle_);
+            completeCycle(to, move.type());
+            inCycle_ = false;
+            continue;
+        }
+
+        if (move.isCycleBegin()) {
+            JS_ASSERT(!inCycle_);
 
             // Characterize the cycle.
             bool allGeneralRegs = true, allFloatRegs = true;
@@ -123,7 +123,7 @@ MoveEmitterX86::emit(const MoveResolver &moves)
             }
 
             // Otherwise use the stack.
-            breakCycle(to, move.type());
+            breakCycle(to, move.endCycleType());
             inCycle_ = true;
         }
 
@@ -256,7 +256,9 @@ MoveEmitterX86::breakCycle(const MoveOperand &to, MoveOp::Type type)
       case MoveOp::INT32:
 #endif
       case MoveOp::GENERAL:
+        JS_ASSERT(pushedAtCycle_ == -1);
         masm.Push(toOperand(to));
+        pushedAtCycle_ = masm.framePushed();
         break;
       default:
         MOZ_ASSUME_UNREACHABLE("Unexpected move type");
@@ -266,6 +268,8 @@ MoveEmitterX86::breakCycle(const MoveOperand &to, MoveOp::Type type)
 void
 MoveEmitterX86::completeCycle(const MoveOperand &to, MoveOp::Type type)
 {
+    JS_ASSERT(pushedAtCycle_ != -1);
+
     // There is some pattern:
     //   (A -> B)
     //   (B -> A)
@@ -274,6 +278,7 @@ MoveEmitterX86::completeCycle(const MoveOperand &to, MoveOp::Type type)
     // saved value of B, to A.
     switch (type) {
       case MoveOp::FLOAT32:
+        JS_ASSERT(pushedAtCycle_ - pushedAtStart_ >= sizeof(float));
         if (to.isMemory()) {
             masm.loadFloat32(cycleSlot(), ScratchFloatReg);
             masm.storeFloat32(ScratchFloatReg, toAddress(to));
@@ -282,6 +287,7 @@ MoveEmitterX86::completeCycle(const MoveOperand &to, MoveOp::Type type)
         }
         break;
       case MoveOp::DOUBLE:
+        JS_ASSERT(pushedAtCycle_ - pushedAtStart_ >= sizeof(double));
         if (to.isMemory()) {
             masm.loadDouble(cycleSlot(), ScratchFloatReg);
             masm.storeDouble(ScratchFloatReg, toAddress(to));
@@ -291,6 +297,7 @@ MoveEmitterX86::completeCycle(const MoveOperand &to, MoveOp::Type type)
         break;
 #ifdef JS_CPU_X64
       case MoveOp::INT32:
+        JS_ASSERT(pushedAtCycle_ - pushedAtStart_ >= sizeof(int32_t));
         // x64 can't pop to a 32-bit destination.
         if (to.isMemory()) {
             masm.load32(cycleSlot(), ScratchReg);
@@ -304,11 +311,13 @@ MoveEmitterX86::completeCycle(const MoveOperand &to, MoveOp::Type type)
       case MoveOp::INT32:
 #endif
       case MoveOp::GENERAL:
+        JS_ASSERT(pushedAtCycle_ - pushedAtStart_ >= sizeof(intptr_t));
         if (to.isMemory()) {
             masm.Pop(toPopOperand(to));
         } else {
             masm.Pop(to.reg());
         }
+        pushedAtCycle_ = -1;
         break;
       default:
         MOZ_ASSUME_UNREACHABLE("Unexpected move type");