Bug 861165: IonMonkey: Correct behaviour for JSOP_IN on dense native with negative index, r=bhackett
☠☠ backed out by b619d1769047 ☠ ☠
authorHannes Verschore <hv1989@gmail.com>
Wed, 17 Apr 2013 11:31:45 +0200
changeset 129036 a0016de79bf9237674149b8d238548419757e1f0
parent 129035 11458a5db07f2d4e10ce989ea385474872435cea
child 129037 0e7c67a08b56b91d559c9a769a9fb193f0ab749c
push id26635
push userhv1989@gmail.com
push dateWed, 17 Apr 2013 09:33:36 +0000
treeherdermozilla-inbound@862431c42e72 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbhackett
bugs861165
milestone23.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 861165: IonMonkey: Correct behaviour for JSOP_IN on dense native with negative index, r=bhackett
js/src/ion/CodeGenerator.cpp
js/src/ion/IonBuilder.cpp
js/src/ion/LIR-Common.h
js/src/ion/Lowering.cpp
js/src/ion/MIR.cpp
js/src/ion/MIR.h
js/src/ion/TypePolicy.cpp
js/src/ion/VMFunctions.cpp
js/src/ion/VMFunctions.h
js/src/jit-test/tests/ion/bug861165.js
--- a/js/src/ion/CodeGenerator.cpp
+++ b/js/src/ion/CodeGenerator.cpp
@@ -5556,45 +5556,86 @@ bool
 CodeGenerator::visitIn(LIn *ins)
 {
     pushArg(ToRegister(ins->rhs()));
     pushArg(ToValue(ins, LIn::LHS));
 
     return callVM(OperatorInInfo, ins);
 }
 
+typedef bool (*OperatorInIFn)(JSContext *, uint32_t, HandleObject, JSBool *);
+static const VMFunction OperatorInIInfo = FunctionInfo<OperatorInIFn>(OperatorInI);
+
 bool
 CodeGenerator::visitInArray(LInArray *lir)
 {
+    const MInArray *mir = lir->mir();
     Register elements = ToRegister(lir->elements());
     Register initLength = ToRegister(lir->initLength());
     Register output = ToRegister(lir->output());
 
     // When the array is not packed we need to do a hole check in addition to the bounds check.
-    Label falseBranch, done;
+    Label falseBranch, done, trueBranch;
+
+    OutOfLineCode *ool = NULL;
+    Label* failedInitLength = &falseBranch;
+
     if (lir->index()->isConstant()) {
-        masm.branch32(Assembler::BelowOrEqual, initLength, Imm32(ToInt32(lir->index())), &falseBranch);
-        if (lir->mir()->needsHoleCheck()) {
-            masm.branchTestMagic(Assembler::Equal, Address(elements, ToInt32(lir->index()) * sizeof(Value)),
-                                 &falseBranch);
+        int32_t index = ToInt32(lir->index());
+
+        JS_ASSERT_IF(index < 0, mir->needsNegativeIntCheck());
+        if (mir->needsNegativeIntCheck()) {
+            ool = oolCallVM(OperatorInIInfo, lir,
+                            (ArgList(), Imm32(index), ToRegister(lir->object())),
+                            StoreRegisterTo(output));
+            failedInitLength = ool->entry();
+        }
+
+        masm.branch32(Assembler::BelowOrEqual, initLength, Imm32(index), failedInitLength);
+        if (mir->needsHoleCheck()) {
+            Address address = Address(elements, index * sizeof(Value));
+            masm.branchTestMagic(Assembler::Equal, address, &falseBranch);
         }
     } else {
-        masm.branch32(Assembler::BelowOrEqual, initLength, ToRegister(lir->index()), &falseBranch);
-        if (lir->mir()->needsHoleCheck()) {
-            masm.branchTestMagic(Assembler::Equal, BaseIndex(elements, ToRegister(lir->index()), TimesEight),
-                                 &falseBranch);
+        Label negativeIntCheck;
+        Register index = ToRegister(lir->index());
+
+        if (mir->needsNegativeIntCheck())
+            failedInitLength = &negativeIntCheck;
+
+        masm.branch32(Assembler::BelowOrEqual, initLength, index, failedInitLength);
+        if (mir->needsHoleCheck()) {
+            BaseIndex address = BaseIndex(elements, ToRegister(lir->index()), TimesEight);
+            masm.branchTestMagic(Assembler::Equal, address, &falseBranch);
+        }
+        masm.jump(&trueBranch);
+
+        if (mir->needsNegativeIntCheck()) {
+            masm.bind(&negativeIntCheck);
+            ool = oolCallVM(OperatorInIInfo, lir,
+                            (ArgList(), index, ToRegister(lir->object())),
+                            StoreRegisterTo(output));
+
+            masm.testl(index, index);
+            masm.j(Assembler::Signed, ool->entry());
+            masm.jump(&falseBranch);
         }
     }
 
+    masm.bind(&trueBranch);
     masm.move32(Imm32(1), output);
     masm.jump(&done);
 
     masm.bind(&falseBranch);
     masm.move32(Imm32(0), output);
     masm.bind(&done);
+
+    if (ool)
+        masm.bind(ool->rejoin());
+
     return true;
 }
 
 bool
 CodeGenerator::visitInstanceOfO(LInstanceOfO *ins)
 {
     return emitInstanceOf(ins, ins->mir()->prototypeObject());
 }
--- a/js/src/ion/IonBuilder.cpp
+++ b/js/src/ion/IonBuilder.cpp
@@ -7389,17 +7389,17 @@ IonBuilder::jsop_in_dense()
     // Get the elements vector.
     MElements *elements = MElements::New(obj);
     current->add(elements);
 
     MInitializedLength *initLength = MInitializedLength::New(elements);
     current->add(initLength);
 
     // Check if id < initLength and elem[id] not a hole.
-    MInArray *ins = MInArray::New(elements, id, initLength, needsHoleCheck);
+    MInArray *ins = MInArray::New(elements, id, initLength, obj, needsHoleCheck);
 
     current->add(ins);
     current->push(ins);
 
     return true;
 }
 
 bool
--- a/js/src/ion/LIR-Common.h
+++ b/js/src/ion/LIR-Common.h
@@ -2713,38 +2713,44 @@ class LLoadElementV : public LInstructio
     const LAllocation *elements() {
         return getOperand(0);
     }
     const LAllocation *index() {
         return getOperand(1);
     }
 };
 
-class LInArray : public LInstructionHelper<1, 3, 0>
+class LInArray : public LInstructionHelper<1, 4, 0>
 {
   public:
     LIR_HEADER(InArray)
 
-    LInArray(const LAllocation &elements, const LAllocation &index, const LAllocation &initLength) {
+    LInArray(const LAllocation &elements, const LAllocation &index,
+             const LAllocation &initLength, const LAllocation &object)
+    {
         setOperand(0, elements);
         setOperand(1, index);
         setOperand(2, initLength);
+        setOperand(3, object);
     }
     const MInArray *mir() const {
         return mir_->toInArray();
     }
     const LAllocation *elements() {
         return getOperand(0);
     }
     const LAllocation *index() {
         return getOperand(1);
     }
     const LAllocation *initLength() {
         return getOperand(2);
     }
+    const LAllocation *object() {
+        return getOperand(3);
+    }
 };
 
 
 // Load a value from a dense array's elements vector. Bail out if it's the hole value.
 class LLoadElementHole : public LInstructionHelper<BOX_PIECES, 3, 0>
 {
   public:
     LIR_HEADER(LoadElementHole)
--- a/js/src/ion/Lowering.cpp
+++ b/js/src/ion/Lowering.cpp
@@ -1814,21 +1814,29 @@ LIRGenerator::visitBoundsCheckLower(MBou
 }
 
 bool
 LIRGenerator::visitInArray(MInArray *ins)
 {
     JS_ASSERT(ins->elements()->type() == MIRType_Elements);
     JS_ASSERT(ins->index()->type() == MIRType_Int32);
     JS_ASSERT(ins->initLength()->type() == MIRType_Int32);
+    JS_ASSERT(ins->object()->type() == MIRType_Object);
     JS_ASSERT(ins->type() == MIRType_Boolean);
 
+    LAllocation object;
+    if (ins->needsNegativeIntCheck())
+        object = useRegister(ins->object());
+    else
+        object = LConstantIndex::Bogus();
+
     LInArray *lir = new LInArray(useRegister(ins->elements()),
                                  useRegisterOrConstant(ins->index()),
-                                 useRegister(ins->initLength()));
+                                 useRegister(ins->initLength()),
+                                 object);
     return define(lir, ins) && assignSafepoint(lir, ins);
 }
 
 bool
 LIRGenerator::visitLoadElement(MLoadElement *ins)
 {
     JS_ASSERT(ins->elements()->type() == MIRType_Elements);
     JS_ASSERT(ins->index()->type() == MIRType_Int32);
--- a/js/src/ion/MIR.cpp
+++ b/js/src/ion/MIR.cpp
@@ -2002,16 +2002,22 @@ InlinePropertyTable::hasFunction(JSFunct
 {
     for (size_t i = 0; i < numEntries(); i++) {
         if (entries_[i]->func == func)
             return true;
     }
     return false;
 }
 
+bool
+MInArray::needsNegativeIntCheck() const
+{
+    return !index()->range() || index()->range()->lower() < 0;
+}
+
 MDefinition *
 MAsmJSUnsignedToDouble::foldsTo(bool useValueNumbers)
 {
     if (input()->isConstant()) {
         const Value &v = input()->toConstant()->value();
         if (v.isInt32())
             return MConstant::New(DoubleValue(uint32_t(v.toInt32())));
     }
--- a/js/src/ion/MIR.h
+++ b/js/src/ion/MIR.h
@@ -1649,16 +1649,67 @@ class MTernaryInstruction : public MAryI
         MDefinition *insThird = ins->getOperand(2);
 
         return first->valueNumber() == insFirst->valueNumber() &&
                second->valueNumber() == insSecond->valueNumber() &&
                third->valueNumber() == insThird->valueNumber();
     }
 };
 
+class MQuaternaryInstruction : public MAryInstruction<4>
+{
+  protected:
+    MQuaternaryInstruction(MDefinition *first, MDefinition *second,
+                           MDefinition *third, MDefinition *fourth)
+    {
+        setOperand(0, first);
+        setOperand(1, second);
+        setOperand(2, third);
+        setOperand(3, fourth);
+    }
+
+  protected:
+    HashNumber valueHash() const
+    {
+        MDefinition *first = getOperand(0);
+        MDefinition *second = getOperand(1);
+        MDefinition *third = getOperand(2);
+        MDefinition *fourth = getOperand(3);
+
+        return op() ^ first->valueNumber() ^ second->valueNumber() ^
+                      third->valueNumber() ^ fourth->valueNumber();
+    }
+
+    bool congruentTo(MDefinition *const &ins) const
+    {
+        if (op() != ins->op())
+            return false;
+
+        if (type() != ins->type())
+            return false;
+
+        if (isEffectful() || ins->isEffectful())
+            return false;
+
+        MDefinition *first = getOperand(0);
+        MDefinition *second = getOperand(1);
+        MDefinition *third = getOperand(2);
+        MDefinition *fourth = getOperand(3);
+        MDefinition *insFirst = ins->getOperand(0);
+        MDefinition *insSecond = ins->getOperand(1);
+        MDefinition *insThird = ins->getOperand(2);
+        MDefinition *insFourth = ins->getOperand(3);
+
+        return first->valueNumber() == insFirst->valueNumber() &&
+               second->valueNumber() == insSecond->valueNumber() &&
+               third->valueNumber() == insThird->valueNumber() &&
+               fourth->valueNumber() == insFourth->valueNumber();
+    }
+};
+
 class MCompare
   : public MBinaryInstruction,
     public ComparePolicy
 {
   public:
     enum CompareType {
 
         // Anything compared to Undefined
@@ -6290,54 +6341,66 @@ class MIn
     TypePolicy *typePolicy() {
         return this;
     }
 };
 
 
 // Test whether the index is in the array bounds or a hole.
 class MInArray
-  : public MTernaryInstruction
+  : public MQuaternaryInstruction,
+    public ObjectPolicy<3>
 {
     bool needsHoleCheck_;
 
-    MInArray(MDefinition *elements, MDefinition *index, MDefinition *initLength, bool needsHoleCheck)
-      : MTernaryInstruction(elements, index, initLength),
+    MInArray(MDefinition *elements, MDefinition *index,
+             MDefinition *initLength, MDefinition *object,
+             bool needsHoleCheck)
+      : MQuaternaryInstruction(elements, index, initLength, object),
         needsHoleCheck_(needsHoleCheck)
     {
         setResultType(MIRType_Boolean);
         setMovable();
         JS_ASSERT(elements->type() == MIRType_Elements);
         JS_ASSERT(index->type() == MIRType_Int32);
         JS_ASSERT(initLength->type() == MIRType_Int32);
     }
 
   public:
     INSTRUCTION_HEADER(InArray)
 
     static MInArray *New(MDefinition *elements, MDefinition *index,
-                         MDefinition *initLength, bool needsHoleCheck) {
-        return new MInArray(elements, index, initLength, needsHoleCheck);
+                         MDefinition *initLength, MDefinition *object,
+                         bool needsHoleCheck)
+    {
+        return new MInArray(elements, index, initLength, object, needsHoleCheck);
     }
 
     MDefinition *elements() const {
         return getOperand(0);
     }
     MDefinition *index() const {
         return getOperand(1);
     }
     MDefinition *initLength() const {
         return getOperand(2);
     }
+    MDefinition *object() const {
+        return getOperand(3);
+    }
     bool needsHoleCheck() const {
         return needsHoleCheck_;
     }
+    bool needsNegativeIntCheck() const;
     AliasSet getAliasSet() const {
         return AliasSet::Load(AliasSet::Element);
     }
+    TypePolicy *typePolicy() {
+        return this;
+    }
 };
 
 // Implementation for instanceof operator with specific rhs.
 class MInstanceOf
   : public MUnaryInstruction,
     public InstanceOfPolicy
 {
     CompilerRootObject protoObj_;
--- a/js/src/ion/TypePolicy.cpp
+++ b/js/src/ion/TypePolicy.cpp
@@ -391,16 +391,18 @@ ObjectPolicy<Op>::staticAdjustInputs(MIn
     MUnbox *replace = MUnbox::New(in, MIRType_Object, MUnbox::Fallible);
     ins->block()->insertBefore(ins, replace);
     ins->replaceOperand(Op, replace);
     return true;
 }
 
 template bool ObjectPolicy<0>::staticAdjustInputs(MInstruction *ins);
 template bool ObjectPolicy<1>::staticAdjustInputs(MInstruction *ins);
+template bool ObjectPolicy<2>::staticAdjustInputs(MInstruction *ins);
+template bool ObjectPolicy<3>::staticAdjustInputs(MInstruction *ins);
 
 bool
 CallPolicy::adjustInputs(MInstruction *ins)
 {
     MCall *call = ins->toCall();
 
     MDefinition *func = call->getFunction();
     if (func->type() == MIRType_Object)
--- a/js/src/ion/VMFunctions.cpp
+++ b/js/src/ion/VMFunctions.cpp
@@ -503,16 +503,23 @@ OperatorIn(JSContext *cx, HandleValue ke
     if (!JSObject::lookupGeneric(cx, obj, id, &obj2, &prop))
         return false;
 
     *out = !!prop;
     return true;
 }
 
 bool
+OperatorInI(JSContext *cx, uint32_t index, HandleObject obj, JSBool *out)
+{
+    RootedValue key(cx, Int32Value(index));
+    return OperatorIn(cx, key, obj, out);
+}
+
+bool
 GetIntrinsicValue(JSContext *cx, HandlePropertyName name, MutableHandleValue rval)
 {
     return cx->global()->getIntrinsicValue(cx, name, rval);
 }
 
 bool
 CreateThis(JSContext *cx, HandleObject callee, MutableHandleValue rval)
 {
--- a/js/src/ion/VMFunctions.h
+++ b/js/src/ion/VMFunctions.h
@@ -516,16 +516,17 @@ bool InterruptCheck(JSContext *cx);
 HeapSlot *NewSlots(JSRuntime *rt, unsigned nslots);
 JSObject *NewCallObject(JSContext *cx, HandleShape shape, HandleTypeObject type, HeapSlot *slots);
 JSObject *NewStringObject(JSContext *cx, HandleString str);
 
 bool SPSEnter(JSContext *cx, HandleScript script);
 bool SPSExit(JSContext *cx, HandleScript script);
 
 bool OperatorIn(JSContext *cx, HandleValue key, HandleObject obj, JSBool *out);
+bool OperatorInI(JSContext *cx, uint32_t index, HandleObject obj, JSBool *out);
 
 bool GetIntrinsicValue(JSContext *cx, HandlePropertyName name, MutableHandleValue rval);
 
 bool CreateThis(JSContext *cx, HandleObject callee, MutableHandleValue rval);
 
 void GetDynamicName(JSContext *cx, JSObject *scopeChain, JSString *str, Value *vp);
 
 JSBool FilterArguments(JSContext *cx, JSString *str);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug861165.js
@@ -0,0 +1,85 @@
+// |jit-test| no-jm
+
+// IM has the following fastpaths:
+// - constant index (constant)
+// - need negative int check (neg)
+// - needs hole check (hole)
+// So to test everything we have to do:
+//            constant | neg | hole
+//  test 1:     0         0      0
+//  test 2:     1         0      0
+//  test 3:     0         1      0
+//  test 4:     1         1      0
+//  test 5:     0         0      1
+//  test 6:     1         0      1
+//  test 7:     0         1      1
+//  test 8:     1         1      1
+
+function test1(index, a) {
+  if (index < 0)
+    index = -index
+  return index in a;
+}
+assertEq(test1(1, [1,2]), true);
+
+function test2(a) {
+  return 0 in a;
+}
+assertEq(test2([1,2]), true);
+
+function test3(index, a) {
+  return index in a;
+}
+
+var arr3 = [];
+arr3["-1073741828"] = 17;
+assertEq(test3(-1073741828, arr3), true);
+
+function test4(a) {
+  return -1073741828 in a;
+}
+assertEq(test4(arr3), true);
+
+
+function test5(index, a) {
+  if (index < 0)
+    index = -index
+  return index in a;
+}
+var arr5 = [];
+arr5[0] = 1
+arr5[1] = 1
+arr5[2] = 1
+arr5[4] = 1
+assertEq(test5(1, arr5), true);
+assertEq(test5(3, arr5), false);
+
+function test7a(a) {
+  return 3 in a;
+}
+function test7b(a) {
+  return 4 in a;
+}
+assertEq(test7a(arr5), false);
+assertEq(test7b(arr5), true);
+
+function test8(index, a) {
+  return index in a;
+}
+arr5["-1073741828"] = 17;
+assertEq(test8(-1073741828, arr5), true);
+assertEq(test8(3, arr5), false);
+assertEq(test8(0, arr5), true);
+
+function test9a(a) {
+  return 0 in a;
+}
+function test9b(a) {
+  return 3 in a;
+}
+function test9c(a) {
+  return -1073741828 in a;
+}
+assertEq(test9a(arr5), true);
+assertEq(test9b(arr5), false);
+assertEq(test9c(arr5), true);