Bug 1167677 - Try harder to find scratch registers for memory->memory MoveGroup moves, r=sunfish.

js/src/jit/BacktrackingAllocator.cpp

@@ -325,14 +325,19 @@ VirtualRegister::setInitialDefinition(CodePosition from)
 }
 
 LiveRange*
-VirtualRegister::rangeFor(CodePosition pos) const
+VirtualRegister::rangeFor(CodePosition pos, bool preferRegister /* = false */) const
 {
+    LiveRange* found = nullptr;
     for (LiveRange::RegisterLinkIterator iter = rangesBegin(); iter; iter++) {
         LiveRange* range = LiveRange::get(*iter);
-        if (range->covers(pos))
-            return range;
+        if (range->covers(pos)) {
+            if (!preferRegister || range->bundle()->allocation().isRegister())
+                return range;
+            if (!found)
+                found = range;
+        }
     }
-    return nullptr;
+    return found;
 }
 
 void
@@ -1706,7 +1711,7 @@ BacktrackingAllocator::resolveControlFlow()
             if (skip)
                 continue;
 
-            LiveRange* predecessorRange = reg.rangeFor(start.previous());
+            LiveRange* predecessorRange = reg.rangeFor(start.previous(), /* preferRegister = */ true);
             if (start.subpos() == CodePosition::INPUT) {
                 if (!moveInput(ins->toInstruction(), predecessorRange, range, reg.type()))
                     return false;
@@ -1742,7 +1747,7 @@ BacktrackingAllocator::resolveControlFlow()
                 MOZ_ASSERT(predecessor->mir()->numSuccessors() == 1);
 
                 LAllocation* input = phi->getOperand(k);
-                LiveRange* from = vreg(input).rangeFor(exitOf(predecessor));
+                LiveRange* from = vreg(input).rangeFor(exitOf(predecessor), /* preferRegister = */ true);
                 MOZ_ASSERT(from);
 
                 if (!moveAtExit(predecessor, from, to, def->type()))
@@ -1767,7 +1772,7 @@ BacktrackingAllocator::resolveControlFlow()
                     if (to->covers(exitOf(predecessor)))
                         continue;
 
-                    LiveRange* from = reg.rangeFor(exitOf(predecessor));
+                    LiveRange* from = reg.rangeFor(exitOf(predecessor), /* preferRegister = */ true);
 
                     if (mSuccessor->numPredecessors() > 1) {
                         MOZ_ASSERT(predecessor->mir()->numSuccessors() == 1);

js/src/jit/BacktrackingAllocator.h

@@ -524,7 +524,7 @@ class VirtualRegister
     LiveRange* lastRange() const {
         return LiveRange::get(ranges_.back());
     }
-    LiveRange* rangeFor(CodePosition pos) const;
+    LiveRange* rangeFor(CodePosition pos, bool preferRegister = false) const;
     void removeRange(LiveRange* range);
     void addRange(LiveRange* range);
 

js/src/jit/CodeGenerator.cpp

@@ -2208,8 +2208,14 @@ CodeGenerator::visitMoveGroup(LMoveGroup* group)
     masm.propagateOOM(resolver.resolve());
 
     MoveEmitter emitter(masm);
+
+#ifdef JS_CODEGEN_X86
     if (group->maybeScratchRegister().isGeneralReg())
         emitter.setScratchRegister(group->maybeScratchRegister().toGeneralReg()->reg());
+    else
+        resolver.sortMemoryToMemoryMoves();
+#endif
+
     emitter.emit(resolver);
     emitter.finish();
 }

js/src/jit/LIR-Common.h

@@ -119,14 +119,10 @@ class LMoveGroup : public LInstructionHelper<0, 0, 0>
     void setScratchRegister(Register reg) {
         scratchRegister_ = LGeneralReg(reg);
     }
-#endif
     LAllocation maybeScratchRegister() {
-#ifdef JS_CODEGEN_X86
         return scratchRegister_;
-#else
-        return LAllocation();
-#endif
     }
+#endif
 
     bool uses(Register reg) {
         for (size_t i = 0; i < numMoves(); i++) {

js/src/jit/MoveResolver.cpp

@@ -201,14 +201,90 @@ MoveResolver::addOrderedMove(const MoveOp& move)
             }
         }
 
-        if (existing.to().aliases(move.from()) ||
-            existing.to().aliases(move.to()) ||
-            existing.from().aliases(move.to()) ||
-            existing.from().aliases(move.from()))
-        {
+        if (existing.aliases(move))
             break;
-        }
     }
 
     return orderedMoves_.append(move);
 }
+
+void
+MoveResolver::reorderMove(size_t from, size_t to)
+{
+    MOZ_ASSERT(from != to);
+
+    MoveOp op = orderedMoves_[from];
+    if (from < to) {
+        for (size_t i = from; i < to; i++)
+            orderedMoves_[i] = orderedMoves_[i + 1];
+    } else {
+        for (size_t i = from; i > to; i--)
+            orderedMoves_[i] = orderedMoves_[i - 1];
+    }
+    orderedMoves_[to] = op;
+}
+
+void
+MoveResolver::sortMemoryToMemoryMoves()
+{
+    // Try to reorder memory->memory moves so that they are executed right
+    // before a move that clobbers some register. This will allow the move
+    // emitter to use that clobbered register as a scratch register for the
+    // memory->memory move, if necessary.
+    for (size_t i = 0; i < orderedMoves_.length(); i++) {
+        const MoveOp& base = orderedMoves_[i];
+        if (!base.from().isMemory() || !base.to().isMemory())
+            continue;
+        if (base.type() != MoveOp::GENERAL && base.type() != MoveOp::INT32)
+            continue;
+
+        // Look for an earlier move clobbering a register.
+        bool found = false;
+        for (int j = i - 1; j >= 0; j--) {
+            const MoveOp& previous = orderedMoves_[j];
+            if (previous.aliases(base) || previous.isCycleBegin() || previous.isCycleEnd())
+                break;
+
+            if (previous.to().isGeneralReg()) {
+                reorderMove(i, j);
+                found = true;
+                break;
+            }
+        }
+        if (found)
+            continue;
+
+        // Look for a later move clobbering a register.
+        if (i + 1 < orderedMoves_.length()) {
+            bool found = false, skippedRegisterUse = false;
+            for (size_t j = i + 1; j < orderedMoves_.length(); j++) {
+                const MoveOp& later = orderedMoves_[j];
+                if (later.aliases(base) || later.isCycleBegin() || later.isCycleEnd())
+                    break;
+
+                if (later.to().isGeneralReg()) {
+                    if (skippedRegisterUse) {
+                        reorderMove(i, j);
+                        found = true;
+                    } else {
+                        // There is no move that uses a register between the
+                        // original memory->memory move and this move that
+                        // clobbers a register. The move should already be able
+                        // to use a scratch register, so don't shift anything
+                        // around.
+                    }
+                    break;
+                }
+
+                if (later.from().isGeneralReg())
+                    skippedRegisterUse = true;
+            }
+
+            if (found) {
+                // Redo the search for memory->memory moves at the current
+                // index, so we don't skip the move just shifted back.
+                i--;
+            }
+        }
+    }
+}

js/src/jit/MoveResolver.h

@@ -197,6 +197,12 @@ class MoveOp
         MOZ_ASSERT(isCycleBegin());
         return endCycleType_;
     }
+    bool aliases(const MoveOperand& op) const {
+        return from().aliases(op) || to().aliases(op);
+    }
+    bool aliases(const MoveOp& other) const {
+        return aliases(other.from()) || aliases(other.to());
+    }
 };
 
 class MoveResolver
@@ -229,8 +235,6 @@ class MoveResolver
     typedef InlineList<MoveResolver::PendingMove>::iterator PendingMoveIterator;
 
   private:
-    // Moves that are definitely unblocked (constants to registers). These are
-    // emitted last.
     js::Vector<MoveOp, 16, SystemAllocPolicy> orderedMoves_;
     int numCycles_;
     int curCycles_;
@@ -241,6 +245,7 @@ class MoveResolver
     PendingMove* findBlockingMove(const PendingMove* last);
     PendingMove* findCycledMove(PendingMoveIterator* stack, PendingMoveIterator end, const PendingMove* first);
     bool addOrderedMove(const MoveOp& move);
+    void reorderMove(size_t from, size_t to);
 
     // Internal reset function. Does not clear lists.
     void resetState();
@@ -257,6 +262,7 @@ class MoveResolver
     // cycle resolution algorithm. Calling addMove() again resets the resolver.
     bool addMove(const MoveOperand& from, const MoveOperand& to, MoveOp::Type type);
     bool resolve();
+    void sortMemoryToMemoryMoves();
 
     size_t numMoves() const {
         return orderedMoves_.length();

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

@@ -11,6 +11,8 @@
 using namespace js;
 using namespace js::jit;
 
+using mozilla::Maybe;
+
 MoveEmitterX86::MoveEmitterX86(MacroAssembler& masm)
   : inCycle_(false),
     masm(masm),
@@ -101,11 +103,19 @@ MoveEmitterX86::emit(const MoveResolver& moves)
 {
 #if defined(JS_CODEGEN_X86) && defined(DEBUG)
     // Clobber any scratch register we have, to make regalloc bugs more visible.
-    if (hasScratchRegister())
-        masm.mov(ImmWord(0xdeadbeef), scratchRegister());
+    if (scratchRegister_.isSome())
+        masm.mov(ImmWord(0xdeadbeef), scratchRegister_.value());
 #endif
 
     for (size_t i = 0; i < moves.numMoves(); i++) {
+#if defined(JS_CODEGEN_X86) && defined(DEBUG)
+        if (!scratchRegister_.isSome()) {
+            Maybe<Register> reg = findScratchRegister(moves, i);
+            if (reg.isSome())
+                masm.mov(ImmWord(0xdeadbeef), reg.value());
+        }
+#endif
+
         const MoveOp& move = moves.getMove(i);
         const MoveOperand& from = move.from();
         const MoveOperand& to = move.to();
@@ -144,10 +154,10 @@ MoveEmitterX86::emit(const MoveResolver& moves)
             emitDoubleMove(from, to);
             break;
           case MoveOp::INT32:
-            emitInt32Move(from, to);
+            emitInt32Move(from, to, moves, i);
             break;
           case MoveOp::GENERAL:
-            emitGeneralMove(from, to);
+            emitGeneralMove(from, to, moves, i);
             break;
           case MoveOp::INT32X4:
             emitInt32X4Move(from, to);
@@ -363,7 +373,8 @@ MoveEmitterX86::completeCycle(const MoveOperand& to, MoveOp::Type type)
 }
 
 void
-MoveEmitterX86::emitInt32Move(const MoveOperand& from, const MoveOperand& to)
+MoveEmitterX86::emitInt32Move(const MoveOperand& from, const MoveOperand& to,
+                              const MoveResolver& moves, size_t i)
 {
     if (from.isGeneralReg()) {
         masm.move32(from.reg(), toOperand(to));
@@ -373,10 +384,10 @@ MoveEmitterX86::emitInt32Move(const MoveOperand& from, const MoveOperand& to)
     } else {
         // Memory to memory gpr move.
         MOZ_ASSERT(from.isMemory());
-        if (hasScratchRegister()) {
-            Register reg = scratchRegister();
-            masm.load32(toAddress(from), reg);
-            masm.move32(reg, toOperand(to));
+        Maybe<Register> reg = findScratchRegister(moves, i);
+        if (reg.isSome()) {
+            masm.load32(toAddress(from), reg.value());
+            masm.move32(reg.value(), toOperand(to));
         } else {
             // No scratch register available; bounce it off the stack.
             masm.Push(toOperand(from));
@@ -386,7 +397,8 @@ MoveEmitterX86::emitInt32Move(const MoveOperand& from, const MoveOperand& to)
 }
 
 void
-MoveEmitterX86::emitGeneralMove(const MoveOperand& from, const MoveOperand& to)
+MoveEmitterX86::emitGeneralMove(const MoveOperand& from, const MoveOperand& to,
+                                const MoveResolver& moves, size_t i)
 {
     if (from.isGeneralReg()) {
         masm.mov(from.reg(), toOperand(to));
@@ -398,10 +410,10 @@ MoveEmitterX86::emitGeneralMove(const MoveOperand& from, const MoveOperand& to)
             masm.lea(toOperand(from), to.reg());
     } else if (from.isMemory()) {
         // Memory to memory gpr move.
-        if (hasScratchRegister()) {
-            Register reg = scratchRegister();
-            masm.loadPtr(toAddress(from), reg);
-            masm.mov(reg, toOperand(to));
+        Maybe<Register> reg = findScratchRegister(moves, i);
+        if (reg.isSome()) {
+            masm.loadPtr(toAddress(from), reg.value());
+            masm.mov(reg.value(), toOperand(to));
         } else {
             // No scratch register available; bounce it off the stack.
             masm.Push(toOperand(from));
@@ -410,10 +422,10 @@ MoveEmitterX86::emitGeneralMove(const MoveOperand& from, const MoveOperand& to)
     } else {
         // Effective address to memory move.
         MOZ_ASSERT(from.isEffectiveAddress());
-        if (hasScratchRegister()) {
-            Register reg = scratchRegister();
-            masm.lea(toOperand(from), reg);
-            masm.mov(reg, toOperand(to));
+        Maybe<Register> reg = findScratchRegister(moves, i);
+        if (reg.isSome()) {
+            masm.lea(toOperand(from), reg.value());
+            masm.mov(reg.value(), toOperand(to));
         } else {
             // This is tricky without a scratch reg. We can't do an lea. Bounce the
             // base register off the stack, then add the offset in place. Note that
@@ -523,3 +535,36 @@ MoveEmitterX86::finish()
     masm.freeStack(masm.framePushed() - pushedAtStart_);
 }
 
+Maybe<Register>
+MoveEmitterX86::findScratchRegister(const MoveResolver& moves, size_t initial)
+{
+#ifdef JS_CODEGEN_X86
+    if (scratchRegister_.isSome())
+        return scratchRegister_;
+
+    /*
+    // All registers are either in use by this move group or are live
+    // afterwards. Look through the remaining moves for a register which is
+    // clobbered before it is used, and is thus dead at this point.
+    AllocatableGeneralRegisterSet regs(GeneralRegisterSet::All());
+    for (size_t i = initial; i < moves.numMoves(); i++) {
+        const MoveOp& move = moves.getMove(i);
+        if (move.from().isGeneralReg())
+            regs.takeUnchecked(move.from().reg());
+        else if (move.from().isMemoryOrEffectiveAddress())
+            regs.takeUnchecked(move.from().base());
+        if (move.to().isGeneralReg()) {
+            if (i != initial && !move.isCycleBegin() && regs.has(move.to().reg()))
+                return mozilla::Some(move.to().reg());
+            regs.takeUnchecked(move.to().reg());
+        } else if (move.to().isMemoryOrEffectiveAddress()) {
+            regs.takeUnchecked(move.to().base());
+        }
+    }
+    */
+
+    return mozilla::Nothing();
+#else
+    return mozilla::Some(ScratchReg);
+#endif
+}

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

@@ -40,8 +40,10 @@ class MoveEmitterX86
                              bool* allGeneralRegs, bool* allFloatRegs);
     bool maybeEmitOptimizedCycle(const MoveResolver& moves, size_t i,
                                  bool allGeneralRegs, bool allFloatRegs, size_t swapCount);
-    void emitInt32Move(const MoveOperand& from, const MoveOperand& to);
-    void emitGeneralMove(const MoveOperand& from, const MoveOperand& to);
+    void emitInt32Move(const MoveOperand& from, const MoveOperand& to,
+                       const MoveResolver& moves, size_t i);
+    void emitGeneralMove(const MoveOperand& from, const MoveOperand& to,
+                         const MoveResolver& moves, size_t i);
     void emitFloat32Move(const MoveOperand& from, const MoveOperand& to);
     void emitDoubleMove(const MoveOperand& from, const MoveOperand& to);
     void emitFloat32X4Move(const MoveOperand& from, const MoveOperand& to);
@@ -61,22 +63,7 @@ class MoveEmitterX86
 #endif
     }
 
-    bool hasScratchRegister() {
-#ifdef JS_CODEGEN_X86
-        return scratchRegister_.isSome();
-#else
-        return true;
-#endif
-    }
-
-    Register scratchRegister() {
-        MOZ_ASSERT(hasScratchRegister());
-#ifdef JS_CODEGEN_X86
-        return scratchRegister_.value();
-#else
-        return ScratchReg;
-#endif
-    }
+    mozilla::Maybe<Register> findScratchRegister(const MoveResolver& moves, size_t i);
 };
 
 typedef MoveEmitterX86 MoveEmitter;