From 0528a958fbf0173f88bfb58b952067bef0aef063 Mon Sep 17 00:00:00 2001 From: Mauricio Collares Neto Date: Sat, 19 Apr 2014 10:37:49 -0700 Subject: [PATCH] Bug 976110 - Part 0: Optimize division and modulus by negative powers of two; r=sunfish --- .../tests/basic/testDivModWithIntMin.js | 19 +++++++ .../jit/shared/CodeGenerator-x86-shared.cpp | 55 +++++++++++++------ js/src/jit/shared/LIR-x86-shared.h | 8 ++- js/src/jit/shared/Lowering-x86-shared.cpp | 22 ++++---- 4 files changed, 73 insertions(+), 31 deletions(-) create mode 100644 js/src/jit-test/tests/basic/testDivModWithIntMin.js diff --git a/js/src/jit-test/tests/basic/testDivModWithIntMin.js b/js/src/jit-test/tests/basic/testDivModWithIntMin.js new file mode 100644 index 00000000000..b81815f546a --- /dev/null +++ b/js/src/jit-test/tests/basic/testDivModWithIntMin.js @@ -0,0 +1,19 @@ +var intMin = -2147483648; + +assertEq(intMin % (-2147483648), -0); +assertEq(intMin % (-3), -2); +assertEq(intMin % (-1), -0); +assertEq(intMin % 1, -0); +assertEq(intMin % 3, -2); +assertEq(intMin % 10, -8); +assertEq(intMin % 2147483647, -1); + +assertEq((-2147483648) % (-2147483648), -0); +for (var i = -10; i <= 10; ++i) + assertEq(i % (-2147483648), i); +assertEq(2147483647 % (-2147483648), 2147483647); + +assertEq((-2147483648) / (-2147483648), 1); +assertEq(0 / (-2147483648), -0); +assertEq((-2147483648) / (-1), 2147483648); +assertEq((-0) / (-2147483648), 0); diff --git a/js/src/jit/shared/CodeGenerator-x86-shared.cpp b/js/src/jit/shared/CodeGenerator-x86-shared.cpp index d3f666f094b..e19ed5c737e 100644 --- a/js/src/jit/shared/CodeGenerator-x86-shared.cpp +++ b/js/src/jit/shared/CodeGenerator-x86-shared.cpp @@ -869,14 +869,23 @@ CodeGeneratorX86Shared::visitDivPowTwoI(LDivPowTwoI *ins) { Register lhs = ToRegister(ins->numerator()); mozilla::DebugOnly output = ToRegister(ins->output()); + int32_t shift = ins->shift(); + bool negativeDivisor = ins->negativeDivisor(); + MDiv *mir = ins->mir(); // We use defineReuseInput so these should always be the same, which is // convenient since all of our instructions here are two-address. JS_ASSERT(lhs == output); + if (!mir->isTruncated() && negativeDivisor) { + // 0 divided by a negative number must return a double. + masm.testl(lhs, lhs); + if (!bailoutIf(Assembler::Zero, ins->snapshot())) + return false; + } + if (shift != 0) { - MDiv *mir = ins->mir(); if (!mir->isTruncated()) { // If the remainder is != 0, bailout since this must be a double. masm.testl(lhs, Imm32(UINT32_MAX >> (32 - shift))); @@ -884,24 +893,26 @@ CodeGeneratorX86Shared::visitDivPowTwoI(LDivPowTwoI *ins) return false; } - if (!mir->canBeNegativeDividend()) { - // Numerator is unsigned, so needs no adjusting. Do the shift. - masm.sarl(Imm32(shift), lhs); - return true; - } - // Adjust the value so that shifting produces a correctly rounded result // when the numerator is negative. See 10-1 "Signed Division by a Known // Power of 2" in Henry S. Warren, Jr.'s Hacker's Delight. - Register lhsCopy = ToRegister(ins->numeratorCopy()); - JS_ASSERT(lhsCopy != lhs); - if (shift > 1) - masm.sarl(Imm32(31), lhs); - masm.shrl(Imm32(32 - shift), lhs); - masm.addl(lhsCopy, lhs); + if (mir->canBeNegativeDividend()) { + Register lhsCopy = ToRegister(ins->numeratorCopy()); + JS_ASSERT(lhsCopy != lhs); + if (shift > 1) + masm.sarl(Imm32(31), lhs); + masm.shrl(Imm32(32 - shift), lhs); + masm.addl(lhsCopy, lhs); + } - // Do the shift. masm.sarl(Imm32(shift), lhs); + if (negativeDivisor) + masm.negl(lhs); + } else if (shift == 0 && negativeDivisor) { + // INT32_MIN / -1 overflows. + masm.negl(lhs); + if (!mir->isTruncated() && !bailoutIf(Assembler::Overflow, ins->snapshot())) + return false; } return true; @@ -1012,7 +1023,7 @@ CodeGeneratorX86Shared::visitModPowTwoI(LModPowTwoI *ins) masm.branchTest32(Assembler::Signed, lhs, lhs, &negative); } - masm.andl(Imm32((1 << shift) - 1), lhs); + masm.andl(Imm32((uint32_t(1) << shift) - 1), lhs); if (ins->mir()->canBeNegativeDividend()) { Label done; @@ -1020,11 +1031,19 @@ CodeGeneratorX86Shared::visitModPowTwoI(LModPowTwoI *ins) // Negative numbers need a negate, bitmask, negate masm.bind(&negative); - // visitModI has an overflow check here to catch INT_MIN % -1, but - // here the rhs is a power of 2, and cannot be -1, so the check is not generated. + + // Unlike in the visitModI case, we are not computing the mod by means of a + // division. Therefore, the divisor = -1 case isn't problematic (the andl + // always returns 0, which is what we expect). + // + // The negl instruction overflows if lhs == INT32_MIN, but this is also not + // a problem: shift is at most 31, and so the andl also always returns 0. masm.negl(lhs); - masm.andl(Imm32((1 << shift) - 1), lhs); + masm.andl(Imm32((uint32_t(1) << shift) - 1), lhs); masm.negl(lhs); + + // Since a%b has the same sign as b, and a is negative in this branch, + // an answer of 0 means the correct result is actually -0. Bail out. if (!ins->mir()->isTruncated() && !bailoutIf(Assembler::Zero, ins->snapshot())) return false; masm.bind(&done); diff --git a/js/src/jit/shared/LIR-x86-shared.h b/js/src/jit/shared/LIR-x86-shared.h index 53dfc7ce801..35d4c0e3996 100644 --- a/js/src/jit/shared/LIR-x86-shared.h +++ b/js/src/jit/shared/LIR-x86-shared.h @@ -47,12 +47,13 @@ class LDivI : public LBinaryMath<1> class LDivPowTwoI : public LBinaryMath<0> { const int32_t shift_; + const bool negativeDivisor_; public: LIR_HEADER(DivPowTwoI) - LDivPowTwoI(const LAllocation &lhs, const LAllocation &lhsCopy, int32_t shift) - : shift_(shift) + LDivPowTwoI(const LAllocation &lhs, const LAllocation &lhsCopy, int32_t shift, bool negativeDivisor) + : shift_(shift), negativeDivisor_(negativeDivisor) { setOperand(0, lhs); setOperand(1, lhsCopy); @@ -67,6 +68,9 @@ class LDivPowTwoI : public LBinaryMath<0> int32_t shift() const { return shift_; } + bool negativeDivisor() const { + return negativeDivisor_; + } MDiv *mir() const { return mir_->toDiv(); } diff --git a/js/src/jit/shared/Lowering-x86-shared.cpp b/js/src/jit/shared/Lowering-x86-shared.cpp index 684fc7c77b6..6fe112d2988 100644 --- a/js/src/jit/shared/Lowering-x86-shared.cpp +++ b/js/src/jit/shared/Lowering-x86-shared.cpp @@ -15,6 +15,7 @@ using namespace js; using namespace js::jit; +using mozilla::Abs; using mozilla::FloorLog2; LTableSwitch * @@ -137,22 +138,21 @@ LIRGeneratorX86Shared::lowerDivI(MDiv *div) if (div->rhs()->isConstant()) { int32_t rhs = div->rhs()->toConstant()->value().toInt32(); - // Check for division by a positive power of two, which is an easy and - // important case to optimize. Note that other optimizations are also - // possible; division by negative powers of two can be optimized in a - // similar manner as positive powers of two, and division by other - // constants can be optimized by a reciprocal multiplication technique. - int32_t shift = FloorLog2(rhs); - if (rhs > 0 && 1 << shift == rhs) { + // Check for division by a power of two, which is an easy and + // important case to optimize. Note that other optimizations + // are also possible: division by other constants can be + // optimized by a reciprocal multiplication technique. + int32_t shift = FloorLog2(Abs(rhs)); + if (rhs != 0 && uint32_t(1) << shift == Abs(rhs)) { LAllocation lhs = useRegisterAtStart(div->lhs()); LDivPowTwoI *lir; if (!div->canBeNegativeDividend()) { // Numerator is unsigned, so does not need adjusting. - lir = new(alloc()) LDivPowTwoI(lhs, lhs, shift); + lir = new(alloc()) LDivPowTwoI(lhs, lhs, shift, rhs < 0); } else { // Numerator is signed, and needs adjusting, and an extra // lhs copy register is needed. - lir = new(alloc()) LDivPowTwoI(lhs, useRegister(div->lhs()), shift); + lir = new(alloc()) LDivPowTwoI(lhs, useRegister(div->lhs()), shift, rhs < 0); } if (div->fallible() && !assignSnapshot(lir, Bailout_BaselineInfo)) return false; @@ -175,8 +175,8 @@ LIRGeneratorX86Shared::lowerModI(MMod *mod) if (mod->rhs()->isConstant()) { int32_t rhs = mod->rhs()->toConstant()->value().toInt32(); - int32_t shift = FloorLog2(rhs); - if (rhs > 0 && 1 << shift == rhs) { + int32_t shift = FloorLog2(Abs(rhs)); + if (rhs != 0 && uint32_t(1) << shift == Abs(rhs)) { LModPowTwoI *lir = new(alloc()) LModPowTwoI(useRegisterAtStart(mod->lhs()), shift); if (mod->fallible() && !assignSnapshot(lir, Bailout_BaselineInfo)) return false;