Don't bailout if MulI results in positive zero (bug 716504, r=dvander,mjrosenb)
authorJan de Mooij <jdemooij@mozilla.com>
Tue, 10 Jan 2012 16:32:49 +0100
changeset 84775 23ce5e04429d0d69f5bffec07965d96f4fe21889
parent 84774 6a3adb008b1e877e5954b999aca21a9ddf4e5757
child 84776 6caa08de3522595ffec4ab20d1b0cb4a348b3ad9
child 84859 b1fcb67fde3fbec5185693c1b46db22db65f9830
push id451
push userjandemooij@gmail.com
push dateTue, 10 Jan 2012 15:35:51 +0000
reviewersdvander, mjrosenb
bugs716504
milestone12.0a1
Don't bailout if MulI results in positive zero (bug 716504, r=dvander,mjrosenb)
js/src/ion/LIR-Common.h
js/src/ion/Lowering.cpp
js/src/ion/arm/CodeGenerator-arm.cpp
js/src/ion/arm/LIR-arm.h
js/src/ion/arm/Lowering-arm.cpp
js/src/ion/arm/Lowering-arm.h
js/src/ion/shared/CodeGenerator-x86-shared.cpp
js/src/ion/shared/CodeGenerator-x86-shared.h
js/src/ion/shared/LIR-x86-shared.h
js/src/ion/shared/Lowering-x86-shared.cpp
js/src/ion/shared/Lowering-x86-shared.h
js/src/jit-test/tests/ion/bug716504.js
--- a/js/src/ion/LIR-Common.h
+++ b/js/src/ion/LIR-Common.h
@@ -551,18 +551,18 @@ class LShiftOp : public LInstructionHelp
 // Returns from the function being compiled (not used in inlined frames). The
 // input must be a box.
 class LReturn : public LInstructionHelper<0, BOX_PIECES, 0>
 {
   public:
     LIR_HEADER(Return);
 };
 
-template <size_t Temps>
-class LBinaryMath : public LInstructionHelper<1, 2, Temps>
+template <size_t Temps, size_t ExtraUses = 0>
+class LBinaryMath : public LInstructionHelper<1, 2 + ExtraUses, Temps>
 {
   public:
     const LAllocation *lhs() {
         return this->getOperand(0);
     }
     const LAllocation *rhs() {
         return this->getOperand(1);
     }
@@ -580,27 +580,16 @@ class LAddI : public LBinaryMath<0>
 
 // Subtracts two integers, returning an integer value.
 class LSubI : public LBinaryMath<0>
 {
   public:
     LIR_HEADER(SubI);
 };
 
-// Adds two integers, returning an integer value.
-class LMulI : public LBinaryMath<0>
-{
-  public:
-    LIR_HEADER(MulI);
-
-    MMul *mir() {
-        return mir_->toMul();
-    }
-};
-
 // Performs an add, sub, mul, or div on two double values.
 class LMathD : public LBinaryMath<0>
 {
     JSOp jsop_;
 
   public:
     LIR_HEADER(MathD);
 
--- a/js/src/ion/Lowering.cpp
+++ b/js/src/ion/Lowering.cpp
@@ -425,20 +425,17 @@ LIRGenerator::visitMul(MMul *ins)
 {
     MDefinition *lhs = ins->lhs();
     MDefinition *rhs = ins->rhs();
     JS_ASSERT(lhs->type() == rhs->type());
 
     if (ins->specialization() == MIRType_Int32) {
         JS_ASSERT(lhs->type() == MIRType_Int32);
         ReorderCommutative(&lhs, &rhs);
-        LMulI *lir = new LMulI;
-        if (ins->fallible() && !assignSnapshot(lir))
-            return false;
-        return lowerForALU(lir, ins, lhs, rhs);
+        return lowerMulI(ins, lhs, rhs);
     }
     if (ins->specialization() == MIRType_Double) {
         JS_ASSERT(lhs->type() == MIRType_Double);
         return lowerForFPU(new LMathD(JSOP_MUL), ins, lhs, rhs);
     }
 
     JS_NOT_REACHED("NYI");
     return false;
--- a/js/src/ion/arm/CodeGenerator-arm.cpp
+++ b/js/src/ion/arm/CodeGenerator-arm.cpp
@@ -458,26 +458,33 @@ CodeGeneratorARM::visitMulI(LMulI *ins)
             masm.ma_mul(ToRegister(lhs), ToRegister(rhs), ToRegister(dest));
         }
 
 
         // Bailout on overflow
         if (mul->canOverflow() && !bailoutIf(c, ins->snapshot()))
             return false;
 
-        // Bailout on 0 (could be -0.0)
         if (mul->canBeNegativeZero()) {
-            masm.ma_cmp(ToRegister(lhs), Imm32(0));
-            if (!bailoutIf(Assembler::Zero, ins->snapshot()))
+            Label done;
+            masm.ma_cmp(ToRegister(dest), Imm32(0));
+            masm.ma_b(&done, Assembler::NotEqual);
+
+            // Result is -0 if lhs or rhs is negative.
+            masm.ma_cmn(ToRegister(lhs), ToRegister(rhs));
+            if (!bailoutIf(Assembler::Signed, ins->snapshot()))
                 return false;
+
+            masm.bind(&done);
         }
     }
 
     return true;
 }
+
 extern "C" {
     extern int __aeabi_idiv(int,int);
 }
 bool
 CodeGeneratorARM::visitDivI(LDivI *ins)
 {
     // Extract the registers from this instruction
     Register lhs = ToRegister(ins->lhs());
--- a/js/src/ion/arm/LIR-arm.h
+++ b/js/src/ion/arm/LIR-arm.h
@@ -203,13 +203,23 @@ class LRecompileCheck : public LInstruct
     LRecompileCheck(const LDefinition &temp) {
         setTemp(0, temp);
     }
     const LAllocation *tempInt() {
         return getTemp(0)->output();
     }
 };
 
+class LMulI : public LBinaryMath<0>
+{
+  public:
+    LIR_HEADER(MulI);
+
+    MMul *mir() {
+        return mir_->toMul();
+    }
+};
+
 } // namespace ion
 } // namespace js
 
 #endif // jsion_lir_arm_h__
 
--- a/js/src/ion/arm/Lowering-arm.cpp
+++ b/js/src/ion/arm/Lowering-arm.cpp
@@ -279,16 +279,25 @@ LIRGeneratorARM::lowerDivI(MDiv *div)
                            tempFixed(r2), tempFixed(r3)/*, tempFixed(lr)*/);
     
     return define(lir, div,
                   LDefinition(LDefinition::TypeFrom(div->type()), LDefinition::DEFAULT))
     && assignSnapshot(lir);
 }
 
 bool
+LIRGeneratorARM::lowerMulI(MMul *mul, MDefinition *lhs, MDefinition *rhs)
+{
+    LMulI *lir = new LMulI;
+    if (mul->fallible() && !assignSnapshot(lir))
+        return false;
+    return lowerForALU(lir, mul, lhs, rhs);
+}
+
+bool
 LIRGeneratorARM::lowerModI(MMod *mod)
 {
     JS_NOT_REACHED("NYI: LModI");
     return false;
 }
 
 bool
 LIRGeneratorARM::visitTableSwitch(MTableSwitch *tableswitch)
--- a/js/src/ion/arm/Lowering-arm.h
+++ b/js/src/ion/arm/Lowering-arm.h
@@ -73,16 +73,17 @@ class LIRGeneratorARM : public LIRGenera
     bool lowerForFPU(LInstructionHelper<1, 1, 0> *ins, MDefinition *mir,
                      MDefinition *src);
     bool lowerForFPU(LInstructionHelper<1, 2, 0> *ins, MDefinition *mir,
                      MDefinition *lhs, MDefinition *rhs);
 
     bool lowerConstantDouble(double d, MInstruction *ins);
     bool lowerDivI(MDiv *div);
     bool lowerModI(MMod *mod);
+    bool lowerMulI(MMul *mul, MDefinition *lhs, MDefinition *rhs);
     bool visitTableSwitch(MTableSwitch *tableswitch);
 
   public:
     bool visitConstant(MConstant *ins);
     bool visitBox(MBox *box);
     bool visitUnbox(MUnbox *unbox);
     bool visitReturn(MReturn *ret);
     bool lowerPhi(MPhi *phi);
--- a/js/src/ion/shared/CodeGenerator-x86-shared.cpp
+++ b/js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ -44,16 +44,19 @@
 #include "CodeGenerator-shared-inl.h"
 #include "ion/IonFrames.h"
 #include "ion/MoveEmitter.h"
 #include "ion/IonCompartment.h"
 
 using namespace js;
 using namespace js::ion;
 
+namespace js {
+namespace ion {
+
 class DeferredJumpTable : public DeferredData
 {
     LTableSwitch *lswitch;
 
   public:
     DeferredJumpTable(LTableSwitch *lswitch)
       : lswitch(lswitch)
     { }
@@ -404,16 +407,33 @@ CodeGeneratorX86Shared::visitSubI(LSubI 
     else
         masm.subl(ToOperand(ins->rhs()), ToRegister(ins->lhs()));
 
     if (ins->snapshot() && !bailoutIf(Assembler::Overflow, ins->snapshot()))
         return false;
     return true;
 }
 
+class MulNegativeZeroCheck : public OutOfLineCodeBase<CodeGeneratorX86Shared>
+{
+    LMulI *ins_;
+
+  public:
+    MulNegativeZeroCheck(LMulI *ins)
+      : ins_(ins)
+    { }
+
+    virtual bool accept(CodeGeneratorX86Shared *codegen) {
+        return codegen->visitMulNegativeZeroCheck(this);
+    }
+    LMulI *ins() const {
+        return ins_;
+    }
+};
+
 bool
 CodeGeneratorX86Shared::visitMulI(LMulI *ins)
 {
     const LAllocation *lhs = ins->lhs();
     const LAllocation *rhs = ins->rhs();
     MMul *mul = ins->mir();
 
     if (rhs->isConstant()) {
@@ -457,28 +477,52 @@ CodeGeneratorX86Shared::visitMulI(LMulI 
             return false;
     } else {
         masm.imull(ToOperand(rhs), ToRegister(lhs));
 
         // Bailout on overflow
         if (mul->canOverflow() && !bailoutIf(Assembler::Overflow, ins->snapshot()))
             return false;
 
-        // Bailout on 0 (could be -0.0)
         if (mul->canBeNegativeZero()) {
+            // Jump to an OOL path if the result is 0.
+            MulNegativeZeroCheck *ool = new MulNegativeZeroCheck(ins);
+            if (!addOutOfLineCode(ool))
+                return false;
+
             masm.testl(ToRegister(lhs), ToRegister(lhs));
-            if (!bailoutIf(Assembler::Equal, ins->snapshot()))
-                return false;
+            masm.j(Assembler::Zero, ool->entry());
+            masm.bind(ool->rejoin());
         }
     }
 
     return true;
 }
 
 bool
+CodeGeneratorX86Shared::visitMulNegativeZeroCheck(MulNegativeZeroCheck *ool)
+{
+    LMulI *ins = ool->ins();
+    Register result = ToRegister(ins->output());
+    Register lhsCopy = ToRegister(ins->lhsCopy());
+    Operand rhs = ToOperand(ins->rhs());
+    JS_ASSERT(lhsCopy != result);
+
+    // Result is -0 if lhs or rhs is negative.
+    masm.movl(lhsCopy, result);
+    masm.orl(rhs, result);
+    if (!bailoutIf(Assembler::Signed, ins->snapshot()))
+        return false;
+
+    masm.xorl(result, result);
+    masm.jmp(ool->rejoin());
+    return true;
+}
+
+bool
 CodeGeneratorX86Shared::visitDivI(LDivI *ins)
 {
     Register remainder = ToRegister(ins->remainder());
     Register lhs = ToRegister(ins->lhs());
     Register rhs = ToRegister(ins->rhs());
 
     JS_ASSERT(remainder == edx);
     JS_ASSERT(lhs == eax);
@@ -856,8 +900,11 @@ CodeGeneratorX86Shared::emitTruncateDoub
 Operand
 CodeGeneratorX86Shared::createArrayElementOperand(Register elements, const LAllocation *index)
 {
     if (index->isConstant())
         return Operand(elements, ToInt32(index) * sizeof(js::Value));
 
     return Operand(elements, ToRegister(index), TimesEight);
 }
+
+} // namespace ion
+} // namespace js
--- a/js/src/ion/shared/CodeGenerator-x86-shared.h
+++ b/js/src/ion/shared/CodeGenerator-x86-shared.h
@@ -43,16 +43,17 @@
 #define jsion_codegen_x86_shared_h__
 
 #include "ion/shared/CodeGenerator-shared.h"
 
 namespace js {
 namespace ion {
 
 class OutOfLineBailout;
+class MulNegativeZeroCheck;
 
 class CodeGeneratorX86Shared : public CodeGeneratorShared
 {
     friend class MoveResolverX86;
 
     CodeGeneratorX86Shared *thisFromCtor() {
         return this;
     }
@@ -131,16 +132,17 @@ class CodeGeneratorX86Shared : public Co
     virtual bool visitMathD(LMathD *math);
     virtual bool visitTableSwitch(LTableSwitch *ins);
     virtual bool visitBoundsCheck(LBoundsCheck *lir);
     virtual bool visitGuardShape(LGuardShape *guard);
     virtual bool visitGuardClass(LGuardClass *guard);
 
     // Out of line visitors.
     bool visitOutOfLineBailout(OutOfLineBailout *ool);
+    bool visitMulNegativeZeroCheck(MulNegativeZeroCheck *ool);
 };
 
 // An out-of-line bailout thunk.
 class OutOfLineBailout : public OutOfLineCodeBase<CodeGeneratorX86Shared>
 {
     LSnapshot *snapshot_;
 
   public:
--- a/js/src/ion/shared/LIR-x86-shared.h
+++ b/js/src/ion/shared/LIR-x86-shared.h
@@ -124,13 +124,37 @@ class LGuardShape : public LInstructionH
 };
 
 class LRecompileCheck : public LInstructionHelper<0, 0, 0>
 {
   public:
     LIR_HEADER(RecompileCheck);
 };
 
+class LMulI : public LBinaryMath<0, 1>
+{
+  public:
+    LIR_HEADER(MulI);
+
+    LMulI(const LAllocation &lhs, const LAllocation &rhs,
+          const LAllocation &lhsCopy)
+    {
+        setOperand(0, lhs);
+        setOperand(1, rhs);
+        setOperand(2, lhsCopy);
+    }
+
+    MMul *mir() {
+        return mir_->toMul();
+    }
+    const LAllocation *lhsCopy() {
+        return this->getOperand(2);
+    }
+    const LDefinition *output() {
+        return this->getDef(0);
+    }
+};
+
 } // namespace ion
 } // namespace js
 
 #endif // jsion_lir_x86_shared_h__
 
--- a/js/src/ion/shared/Lowering-x86-shared.cpp
+++ b/js/src/ion/shared/Lowering-x86-shared.cpp
@@ -78,8 +78,19 @@ LIRGeneratorX86Shared::visitTableSwitch(
 
 bool
 LIRGeneratorX86Shared::visitRecompileCheck(MRecompileCheck *ins)
 {
     LRecompileCheck *lir = new LRecompileCheck();
     return assignSnapshot(lir, Bailout_RecompileCheck) && add(lir);
 }
 
+bool
+LIRGeneratorX86Shared::lowerMulI(MMul *mul, MDefinition *lhs, MDefinition *rhs)
+{
+    // Note: lhs is used twice, so that we can restore the original value for the
+    // negative zero check.
+    LMulI *lir = new LMulI(useRegisterAtStart(lhs), useOrConstant(rhs), useRegister(lhs));
+    if (mul->fallible() && !assignSnapshot(lir))
+        return false;
+    return defineReuseInput(lir, mul, 0);
+}
+
--- a/js/src/ion/shared/Lowering-x86-shared.h
+++ b/js/src/ion/shared/Lowering-x86-shared.h
@@ -48,13 +48,14 @@ namespace ion {
 class LIRGeneratorX86Shared : public LIRGeneratorShared
 {
   protected:
     LIRGeneratorX86Shared(MIRGenerator *gen, MIRGraph &graph, LIRGraph &lirGraph)
       : LIRGeneratorShared(gen, graph, lirGraph)
     {}
     bool visitTableSwitch(MTableSwitch *tableswitch);
     bool visitRecompileCheck(MRecompileCheck *ins);
+    bool lowerMulI(MMul *mul, MDefinition *lhs, MDefinition *rhs);
 };
 
 }
 }
 #endif
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug716504.js
@@ -0,0 +1,10 @@
+function f(x, from, to) {
+    var y = 0;
+    for (var i=from; i<to; i++) {
+        y = i * x;
+    }
+    return y;
+}
+
+assertEq(f(0, 0, 200), 0);
+assertEq(f(0, -10, -5), -0);