From d9aafd1d6a74ae0df1c0bf98ad55632b9d10617e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 18 Dec 2013 08:19:25 -0800 Subject: [PATCH] Bug 951527 - SpiderMonkey: Fix codegen for mixed-type MoveGroup cycles. r=jandem --- js/src/jit/MoveResolver.cpp | 4 +-- js/src/jit/MoveResolver.h | 36 ++++++++++++++++---- js/src/jit/arm/MoveEmitter-arm.cpp | 16 +++++---- js/src/jit/shared/MoveEmitter-x86-shared.cpp | 33 +++++++++++------- 4 files changed, 61 insertions(+), 28 deletions(-) diff --git a/js/src/jit/MoveResolver.cpp b/js/src/jit/MoveResolver.cpp index 8b2c35306dd..af13559631a 100644 --- a/js/src/jit/MoveResolver.cpp +++ b/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); diff --git a/js/src/jit/MoveResolver.h b/js/src/jit/MoveResolver.h index bc87f853d04..0722e1b29cf 100644 --- a/js/src/jit/MoveResolver.h +++ b/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; } }; diff --git a/js/src/jit/arm/MoveEmitter-arm.cpp b/js/src/jit/arm/MoveEmitter-arm.cpp index c06583c7280..0707ec25537 100644 --- a/js/src/jit/arm/MoveEmitter-arm.cpp +++ b/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; } diff --git a/js/src/jit/shared/MoveEmitter-x86-shared.cpp b/js/src/jit/shared/MoveEmitter-x86-shared.cpp index e8143db4ab4..a73c373027f 100644 --- a/js/src/jit/shared/MoveEmitter-x86-shared.cpp +++ b/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");