Undo ADD operations in overflow path, bug 615279.
authorBrian Hackett <bhackett1024@gmail.com>
Mon, 29 Nov 2010 11:26:40 -0800
changeset 74626 e18996c2a36fa23ff2b3e4f27c2d04a55beec622
parent 74625 0581e178dcd8e7093ea6c4ace18d72b73eb0c9a5
child 74627 30ffdc01adf2fb3be916dd0b050593798ef65578
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
bugs615279
milestone2.0b8pre
Undo ADD operations in overflow path, bug 615279.
js/src/jit-test/tests/jaeger/undoAdd.js
js/src/methodjit/FastArithmetic.cpp
js/src/methodjit/FrameState.cpp
js/src/methodjit/FrameState.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/jaeger/undoAdd.js
@@ -0,0 +1,25 @@
+
+/* Test undoing addition in overflow paths when under heavy register pressure. */
+
+function add1(x, y, a, b, res) { var nres = res + 0; var z = (x + a) + (y + b); assertEq(z, nres); }
+function add2(x, y, a, b, res) { var nres = res + 0; var z = (x + a) + (y + b); assertEq(z, nres); }
+function add3(x, y, a, b, res) { var nres = res + 0; var z = (x + a) + (y + b); assertEq(z, nres); }
+add1(0x7ffffff0, 100, 0, 0, 2147483732);
+add2(-1000, -0x80000000, 0, 0, -2147484648);
+add3(-0x80000000, -1000, 0, 0, -2147484648);
+
+function cadd1(x, a, b, res) {
+  var nres = res + 0;
+  var nb = b + 0;
+  var z = (x + a) + 1000;
+  assertEq(z, nres + nb);
+}
+cadd1(0x7ffffff0, 0, 0, 2147484632);
+
+function cadd2(x, a, b, res) {
+  var nres = res + 0;
+  var nb = b + 0;
+  var z = (x + a) + (-0x80000000);
+  assertEq(z, nres + nb);
+}
+cadd2(-1000, 0, 0, -2147484648);
--- a/js/src/methodjit/FastArithmetic.cpp
+++ b/js/src/methodjit/FastArithmetic.cpp
@@ -686,16 +686,29 @@ mjit::Compiler::jsop_binary_full(FrameEn
     /*
      * Integer overflow path. Restore the original values and make a stub call,
      * which could trigger recompilation.
      */
     MaybeJump overflowDone;
     if (preOverflow.isSet())
         stubcc.linkExitDirect(preOverflow.get(), stubcc.masm.label());
     stubcc.linkExitDirect(overflow.get(), stubcc.masm.label());
+
+    /* Restore the original operand registers for ADD. */
+    if (regs.undoResult) {
+        JS_ASSERT(op == JSOP_ADD);
+        if (reg.isSet()) {
+            stubcc.masm.neg32(reg.reg());
+            stubcc.masm.add32(reg.reg(), regs.result);
+            stubcc.masm.neg32(reg.reg());
+        } else {
+            stubcc.masm.add32(Imm32(-value), regs.result);
+        }
+    }
+
     frame.rematBinary(lhs, rhs, regs, stubcc.masm);
     stubcc.syncExitAndJump(Uses(2));
 
     /* The register allocator creates at most one temporary. */
     if (regs.extraFree.isSet())
         frame.freeReg(regs.extraFree.reg());
 
     /* Slow paths funnel here. */
@@ -710,16 +723,24 @@ mjit::Compiler::jsop_binary_full(FrameEn
     /* Slow call - use frame.sync to avoid erroneous jump repatching in stubcc. */
     frame.sync(stubcc.masm, Uses(2));
     stubcc.leave();
     OOL_STUBCALL(stub);
 
     /* Finish up stack operations. */
     frame.popn(2);
 
+    /*
+     * Steal the result register if we remat the LHS/RHS by undoing the operation.
+     * In this case the result register was still assigned to the corresponding
+     * frame entry (so it is synced properly in OOL paths), so steal it back.
+     */
+    if (regs.undoResult)
+        frame.takeReg(regs.result);
+
     if (type == JSVAL_TYPE_INT32)
         frame.pushTypedPayload(type, regs.result);
     else
         frame.pushNumber(regs.result, true);
 
     frame.freeFPReg(regs.lhsFP);
     frame.freeFPReg(regs.rhsFP);
 
--- a/js/src/methodjit/FrameState.cpp
+++ b/js/src/methodjit/FrameState.cpp
@@ -1882,16 +1882,17 @@ FrameState::allocForBinary(FrameEntry *l
             alloc.rhsData = allocReg();
             alloc.extraFree = alloc.rhsData;
             masm.move(Imm32(rhs->getValue().toInt32()), alloc.rhsData.reg());
         }
     }
 
     alloc.lhsNeedsRemat = false;
     alloc.rhsNeedsRemat = false;
+    alloc.undoResult = false;
 
     if (!needsResult)
         goto skip;
 
     /*
      * Now a result register is needed. It must contain a mutable copy of the
      * LHS. For commutative operations, we can opt to use the RHS instead. At
      * this point, if for some reason either must be in a register, that has
@@ -1919,19 +1920,27 @@ FrameState::allocForBinary(FrameEntry *l
         bool leftSynced = backingLeft->data.synced();
         bool rightSynced = backingRight->data.synced();
         if (!commu || (leftInReg && (leftSynced || (!rightInReg || !rightSynced)))) {
             JS_ASSERT(backingLeft->data.inRegister() || !commu);
             JS_ASSERT_IF(backingLeft->data.inRegister(),
                          backingLeft->data.reg() == alloc.lhsData.reg());
             if (backingLeft->data.inRegister()) {
                 alloc.result = backingLeft->data.reg();
-                unpinReg(alloc.result);
-                takeReg(alloc.result);
-                alloc.lhsNeedsRemat = true;
+                if (op == JSOP_ADD && backingLeft == lhs) {
+                    /*
+                     * Leave the operand unsynced, and recover the original operands
+                     * in the OOL path by undoing the operation.
+                     */
+                    alloc.undoResult = true;
+                } else {
+                    unpinReg(alloc.result);
+                    takeReg(alloc.result);
+                    alloc.lhsNeedsRemat = true;
+                }
             } else {
                 /* For now, just spill... */
                 alloc.result = allocReg();
                 masm.move(alloc.lhsData.reg(), alloc.result);
             }
             alloc.resultHasRhs = false;
         } else {
             JS_ASSERT(commu);
--- a/js/src/methodjit/FrameState.h
+++ b/js/src/methodjit/FrameState.h
@@ -466,16 +466,17 @@ class FrameState
         MaybeRegisterID rhsData;
         MaybeRegisterID extraFree;
         RegisterID result;  // mutable result reg
         FPRegisterID lhsFP; // mutable scratch floating point reg
         FPRegisterID rhsFP; // mutable scratch floating point reg
         bool resultHasRhs;  // whether the result has the RHS instead of the LHS
         bool lhsNeedsRemat; // whether LHS needs memory remat
         bool rhsNeedsRemat; // whether RHS needs memory remat
+        bool undoResult;    // whether to remat LHS/RHS by undoing operation
     };
 
     /*
      * Ensures that the two given FrameEntries have registers for both their
      * type and data. The register allocations are returned in a struct.
      *
      * One mutable register is allocated as well, holding the LHS payload. If
      * this would cause a spill that could be avoided by using a mutable RHS,