Bug 925201 - Ensure SetElementIC properly handles dense element holes. (r=shu)
authorEric Faust <efaustbmo@gmail.com>
Wed, 16 Oct 2013 16:37:17 -0700
changeset 165856 79287e1634a4e356b72ce4319946d756459a3f46
parent 165855 8b5aa45e7f762249ebba67338b0d7f0d9138839d
child 165857 53ef1d805b5b3f867ad66628da145382e4acb9f0
push id428
push userbbajaj@mozilla.com
push dateTue, 28 Jan 2014 00:16:25 +0000
treeherdermozilla-release@cd72a7ff3a75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersshu
bugs925201
milestone27.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 925201 - Ensure SetElementIC properly handles dense element holes. (r=shu)
js/src/jit/CodeGenerator.cpp
js/src/jit/CodeGenerator.h
js/src/jit/IonBuilder.cpp
js/src/jit/IonCaches.cpp
js/src/jit/IonCaches.h
js/src/jit/MIR.h
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -6051,25 +6051,27 @@ CodeGenerator::addSetPropertyCache(LInst
       default:
         MOZ_ASSUME_UNREACHABLE("Bad execution mode");
     }
 }
 
 bool
 CodeGenerator::addSetElementCache(LInstruction *ins, Register obj, Register unboxIndex,
                                   Register temp, FloatRegister tempFloat, ValueOperand index,
-                                  ConstantOrRegister value, bool strict)
+                                  ConstantOrRegister value, bool strict, bool guardHoles)
 {
     switch (gen->info().executionMode()) {
       case SequentialExecution: {
-        SetElementIC cache(obj, unboxIndex, temp, tempFloat, index, value, strict);
+        SetElementIC cache(obj, unboxIndex, temp, tempFloat, index, value, strict,
+                           guardHoles);
         return addCache(ins, allocateCache(cache));
       }
       case ParallelExecution: {
-        SetElementParIC cache(obj, unboxIndex, temp, tempFloat, index, value, strict);
+        SetElementParIC cache(obj, unboxIndex, temp, tempFloat, index, value, strict,
+                              guardHoles);
         return addCache(ins, allocateCache(cache));
       }
       default:
         MOZ_ASSUME_UNREACHABLE("Bad execution mode");
     }
 }
 
 bool
@@ -6213,17 +6215,17 @@ CodeGenerator::visitSetElementCacheV(LSe
     Register obj = ToRegister(ins->object());
     Register unboxIndex = ToTempUnboxRegister(ins->tempToUnboxIndex());
     Register temp = ToRegister(ins->temp());
     FloatRegister tempFloat = ToFloatRegister(ins->tempFloat());
     ValueOperand index = ToValue(ins, LSetElementCacheV::Index);
     ConstantOrRegister value = TypedOrValueRegister(ToValue(ins, LSetElementCacheV::Value));
 
     return addSetElementCache(ins, obj, unboxIndex, temp, tempFloat, index, value,
-                              ins->mir()->strict());
+                              ins->mir()->strict(), ins->mir()->guardHoles());
 }
 
 bool
 CodeGenerator::visitSetElementCacheT(LSetElementCacheT *ins)
 {
     Register obj = ToRegister(ins->object());
     Register unboxIndex = ToTempUnboxRegister(ins->tempToUnboxIndex());
     Register temp = ToRegister(ins->temp());
@@ -6232,17 +6234,17 @@ CodeGenerator::visitSetElementCacheT(LSe
     ConstantOrRegister value;
     const LAllocation *tmp = ins->value();
     if (tmp->isConstant())
         value = *tmp->toConstant();
     else
         value = TypedOrValueRegister(ins->mir()->value()->type(), ToAnyRegister(tmp));
 
     return addSetElementCache(ins, obj, unboxIndex, temp, tempFloat, index, value,
-                              ins->mir()->strict());
+                              ins->mir()->strict(), ins->mir()->guardHoles());
 }
 
 typedef bool (*SetElementICFn)(JSContext *, size_t, HandleObject, HandleValue, HandleValue);
 const VMFunction SetElementIC::UpdateInfo =
     FunctionInfo<SetElementICFn>(SetElementIC::update);
 
 bool
 CodeGenerator::visitSetElementIC(OutOfLineUpdateCache *ool, DataPtr<SetElementIC> &ic)
--- a/js/src/jit/CodeGenerator.h
+++ b/js/src/jit/CodeGenerator.h
@@ -338,17 +338,17 @@ class CodeGenerator : public CodeGenerat
                              bool allowGetters);
     bool addGetElementCache(LInstruction *ins, Register obj, ConstantOrRegister index,
                             TypedOrValueRegister output, bool monitoredResult);
     bool addSetPropertyCache(LInstruction *ins, RegisterSet liveRegs, Register objReg,
                              PropertyName *name, ConstantOrRegister value, bool strict,
                              bool needsTypeBarrier);
     bool addSetElementCache(LInstruction *ins, Register obj, Register unboxIndex, Register temp,
                             FloatRegister tempFloat, ValueOperand index, ConstantOrRegister value,
-                            bool strict);
+                            bool strict, bool guardHoles);
     bool checkForAbortPar(LInstruction *lir);
 
     bool generateBranchV(const ValueOperand &value, Label *ifTrue, Label *ifFalse, FloatRegister fr);
 
     bool emitAllocateGCThingPar(LInstruction *lir, const Register &objReg, const Register &sliceReg,
                                 const Register &tempReg1, const Register &tempReg2,
                                 JSObject *templateObj);
 
--- a/js/src/jit/IonBuilder.cpp
+++ b/js/src/jit/IonBuilder.cpp
@@ -7227,18 +7227,24 @@ IonBuilder::setElemTryCache(bool *emitte
         return true;
 
     if (PropertyWriteNeedsTypeBarrier(constraints(), current,
                                       &object, nullptr, &value, /* canModify = */ true))
     {
         return true;
     }
 
+    // We can avoid worrying about holes in the IC if we know a priori we are safe
+    // from them. If TI can guard that there are no indexed properties on the prototype
+    // chain, we know that we anen't missing any setters by overwriting the hole with
+    // another value.
+    bool guardHoles = ElementAccessHasExtraIndexedProperty(constraints(), object);
+
     // Emit SetElementCache.
-    MInstruction *ins = MSetElementCache::New(object, index, value, script()->strict);
+    MInstruction *ins = MSetElementCache::New(object, index, value, script()->strict, guardHoles);
     current->add(ins);
     current->push(value);
 
     if (!resumeAfter(ins))
         return false;
 
     *emitted = true;
     return true;
--- a/js/src/jit/IonCaches.cpp
+++ b/js/src/jit/IonCaches.cpp
@@ -3461,24 +3461,27 @@ IsTypedArrayElementSetInlineable(JSObjec
 {
     // Don't bother attaching stubs for assigning strings and objects.
     return (obj->is<TypedArrayObject>() && idval.isInt32() &&
             !value.isString() && !value.isObject());
 }
 
 static bool
 GenerateSetDenseElement(JSContext *cx, MacroAssembler &masm, IonCache::StubAttacher &attacher,
-                        JSObject *obj, const Value &idval, Register object, ValueOperand indexVal,
-                        ConstantOrRegister value, Register tempToUnboxIndex, Register temp)
+                        JSObject *obj, const Value &idval, bool guardHoles, Register object,
+                        ValueOperand indexVal, ConstantOrRegister value, Register tempToUnboxIndex,
+                        Register temp)
 {
     JS_ASSERT(obj->isNative());
     JS_ASSERT(idval.isInt32());
 
     Label failures;
-    Label outOfBounds; // index >= capacity || index > initialized length
+    Label outOfBounds; // index represents a known hole, or an illegal append
+
+    Label markElem, postBarrier; // used if TI protects us from worrying about holes.
 
     // Guard object is a dense array.
     Shape *shape = obj->lastProperty();
     if (!shape)
         return false;
     masm.branchTestObjShape(Assembler::NotEqual, object, shape, &failures);
 
     // Ensure the index is an int32 value.
@@ -3490,59 +3493,68 @@ GenerateSetDenseElement(JSContext *cx, M
     {
         // Load obj->elements.
         Register elements = temp;
         masm.loadPtr(Address(object, JSObject::offsetOfElements()), elements);
 
         // Compute the location of the element.
         BaseIndex target(elements, index, TimesEight);
 
-        // Guard that we can increase the initialized length.
-        Address capacity(elements, ObjectElements::offsetOfCapacity());
-        masm.branch32(Assembler::BelowOrEqual, capacity, index, &outOfBounds);
-
-        // Guard on the initialized length.
-        Address initLength(elements, ObjectElements::offsetOfInitializedLength());
-        masm.branch32(Assembler::Below, initLength, index, &outOfBounds);
-
-        // if (initLength == index)
-        Label markElem, postBarrier;
-        masm.branch32(Assembler::NotEqual, initLength, index, &markElem);
-        {
-            // Increase initialize length.
-            Int32Key newLength(index);
-            masm.bumpKey(&newLength, 1);
-            masm.storeKey(newLength, initLength);
-
-            // Increase length if needed.
-            Label bumpedLength;
-            Address length(elements, ObjectElements::offsetOfLength());
-            masm.branch32(Assembler::AboveOrEqual, length, index, &bumpedLength);
-            masm.storeKey(newLength, length);
-            masm.bind(&bumpedLength);
-
-            // Restore the index.
-            masm.bumpKey(&newLength, -1);
-            masm.jump(&postBarrier);
+        // If TI cannot help us deal with HOLES by preventing indexed properties
+        // on the prototype chain, we have to be very careful to check for ourselves
+        // to avoid stomping on what should be a setter call. Start by only allowing things
+        // within the initialized length.
+        if (guardHoles) {
+            Address initLength(elements, ObjectElements::offsetOfInitializedLength());
+            masm.branch32(Assembler::BelowOrEqual, initLength, index, &outOfBounds);
+        } else {
+            // Guard that we can increase the initialized length.
+            Address capacity(elements, ObjectElements::offsetOfCapacity());
+            masm.branch32(Assembler::BelowOrEqual, capacity, index, &outOfBounds);
+
+            // Guard on the initialized length.
+            Address initLength(elements, ObjectElements::offsetOfInitializedLength());
+            masm.branch32(Assembler::Below, initLength, index, &outOfBounds);
+
+            // if (initLength == index)
+            masm.branch32(Assembler::NotEqual, initLength, index, &markElem);
+            {
+                // Increase initialize length.
+                Int32Key newLength(index);
+                masm.bumpKey(&newLength, 1);
+                masm.storeKey(newLength, initLength);
+
+                // Increase length if needed.
+                Label bumpedLength;
+                Address length(elements, ObjectElements::offsetOfLength());
+                masm.branch32(Assembler::AboveOrEqual, length, index, &bumpedLength);
+                masm.storeKey(newLength, length);
+                masm.bind(&bumpedLength);
+
+                // Restore the index.
+                masm.bumpKey(&newLength, -1);
+                masm.jump(&postBarrier);
+            }
+            // else
+            masm.bind(&markElem);
         }
-        // else
-        {
-            // Mark old element.
-            masm.bind(&markElem);
-            if (cx->zone()->needsBarrier())
-                masm.callPreBarrier(target, MIRType_Value);
-        }
-
-        // Call post barrier if necessary, and recalculate elements pointer if it got cobbered.
-        masm.bind(&postBarrier);
+
+        if (cx->zone()->needsBarrier())
+            masm.callPreBarrier(target, MIRType_Value);
+
+        // Call post barrier if necessary, and recalculate elements pointer if it got clobbered.
+        if (!guardHoles)
+            masm.bind(&postBarrier);
         Register postBarrierScratch = elements;
         if (masm.maybeCallPostBarrier(object, value, postBarrierScratch))
             masm.loadPtr(Address(object, JSObject::offsetOfElements()), elements);
 
         // Store the value.
+        if (guardHoles)
+            masm.branchTestMagic(Assembler::Equal, target, &failures);
         masm.storeConstantOrRegister(value, target);
     }
     attacher.jumpRejoin(masm);
 
     // All failures flow to here.
     masm.bind(&outOfBounds);
     masm.bind(&failures);
     attacher.jumpNextStub(masm);
@@ -3551,24 +3563,28 @@ GenerateSetDenseElement(JSContext *cx, M
 }
 
 bool
 SetElementIC::attachDenseElement(JSContext *cx, IonScript *ion, JSObject *obj, const Value &idval)
 {
     MacroAssembler masm(cx);
     RepatchStubAppender attacher(*this);
     if (!GenerateSetDenseElement(cx, masm, attacher, obj, idval,
-                                 object(), index(), value(),
-                                 tempToUnboxIndex(), temp()))
+                                 guardHoles(), object(), index(),
+                                 value(), tempToUnboxIndex(),
+                                 temp()))
     {
         return false;
     }
 
     setHasDenseStub();
-    return linkAndAttachStub(cx, masm, attacher, ion, "dense array");
+    const char *message = guardHoles()            ?
+                            "dense array (holes)" :
+                            "dense array";
+    return linkAndAttachStub(cx, masm, attacher, ion, message);
 }
 
 static bool
 GenerateSetTypedArrayElement(JSContext *cx, MacroAssembler &masm, IonCache::StubAttacher &attacher,
                              TypedArrayObject *tarr, Register object,
                              ValueOperand indexVal, ConstantOrRegister value,
                              Register tempUnbox, Register temp, FloatRegister tempFloat)
 {
@@ -3699,23 +3715,28 @@ SetElementIC::reset()
 
 bool
 SetElementParIC::attachDenseElement(LockedJSContext &cx, IonScript *ion, JSObject *obj,
                                     const Value &idval)
 {
     MacroAssembler masm(cx);
     DispatchStubPrepender attacher(*this);
     if (!GenerateSetDenseElement(cx, masm, attacher, obj, idval,
-                                 object(), index(), value(),
-                                 tempToUnboxIndex(), temp()))
+                                 guardHoles(), object(), index(),
+                                 value(), tempToUnboxIndex(),
+                                 temp()))
     {
         return false;
     }
 
-    return linkAndAttachStub(cx, masm, attacher, ion, "parallel dense array");
+    const char *message = guardHoles()                     ?
+                            "parallel dense array (holes)" :
+                            "parallel dense array";
+
+    return linkAndAttachStub(cx, masm, attacher, ion, message);
 }
 
 bool
 SetElementParIC::attachTypedArrayElement(LockedJSContext &cx, IonScript *ion,
                                          TypedArrayObject *tarr)
 {
     MacroAssembler masm(cx);
     DispatchStubPrepender attacher(*this);
--- a/js/src/jit/IonCaches.h
+++ b/js/src/jit/IonCaches.h
@@ -803,30 +803,32 @@ class SetElementIC : public RepatchIonCa
   protected:
     Register object_;
     Register tempToUnboxIndex_;
     Register temp_;
     FloatRegister tempFloat_;
     ValueOperand index_;
     ConstantOrRegister value_;
     bool strict_;
+    bool guardHoles_;
 
     bool hasDenseStub_ : 1;
 
   public:
     SetElementIC(Register object, Register tempToUnboxIndex, Register temp,
                  FloatRegister tempFloat, ValueOperand index, ConstantOrRegister value,
-                 bool strict)
+                 bool strict, bool guardHoles)
       : object_(object),
         tempToUnboxIndex_(tempToUnboxIndex),
         temp_(temp),
         tempFloat_(tempFloat),
         index_(index),
         value_(value),
         strict_(strict),
+        guardHoles_(guardHoles),
         hasDenseStub_(false)
     {
     }
 
     CACHE_HEADER(SetElement)
 
     void reset();
 
@@ -846,16 +848,19 @@ class SetElementIC : public RepatchIonCa
         return index_;
     }
     ConstantOrRegister value() const {
         return value_;
     }
     bool strict() const {
         return strict_;
     }
+    bool guardHoles() const {
+        return guardHoles_;
+    }
 
     bool hasDenseStub() const {
         return hasDenseStub_;
     }
     void setHasDenseStub() {
         JS_ASSERT(!hasDenseStub());
         hasDenseStub_ = true;
     }
@@ -1177,28 +1182,30 @@ class SetElementParIC : public ParallelI
   protected:
     Register object_;
     Register tempToUnboxIndex_;
     Register temp_;
     FloatRegister tempFloat_;
     ValueOperand index_;
     ConstantOrRegister value_;
     bool strict_;
+    bool guardHoles_;
 
   public:
     SetElementParIC(Register object, Register tempToUnboxIndex, Register temp,
                     FloatRegister tempFloat, ValueOperand index, ConstantOrRegister value,
-                    bool strict)
+                    bool strict, bool guardHoles)
       : object_(object),
         tempToUnboxIndex_(tempToUnboxIndex),
         temp_(temp),
         tempFloat_(tempFloat),
         index_(index),
         value_(value),
-        strict_(strict)
+        strict_(strict),
+        guardHoles_(guardHoles)
     {
     }
 
     CACHE_HEADER(SetElementPar)
 
 #ifdef JS_CPU_X86
     // x86 lacks a general purpose scratch register for dispatch caches and
     // must be given one manually.
@@ -1221,16 +1228,19 @@ class SetElementParIC : public ParallelI
         return index_;
     }
     ConstantOrRegister value() const {
         return value_;
     }
     bool strict() const {
         return strict_;
     }
+    bool guardHoles() const {
+        return guardHoles_;
+    }
 
     bool attachDenseElement(LockedJSContext &cx, IonScript *ion, JSObject *obj, const Value &idval);
     bool attachTypedArrayElement(LockedJSContext &cx, IonScript *ion, TypedArrayObject *tarr);
 
     static bool update(ForkJoinSlice *slice, size_t cacheIndex, HandleObject obj,
                        HandleValue idval, HandleValue value);
 };
 
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -7130,34 +7130,40 @@ class MSetPropertyCache
     }
 };
 
 class MSetElementCache
   : public MSetElementInstruction,
     public MixPolicy<ObjectPolicy<0>, BoxPolicy<1> >
 {
     bool strict_;
-
-    MSetElementCache(MDefinition *obj, MDefinition *index, MDefinition *value, bool strict)
+    bool guardHoles_;
+
+    MSetElementCache(MDefinition *obj, MDefinition *index, MDefinition *value, bool strict,
+                     bool guardHoles)
       : MSetElementInstruction(obj, index, value),
-        strict_(strict)
+        strict_(strict),
+        guardHoles_(guardHoles)
     {
     }
 
   public:
     INSTRUCTION_HEADER(SetElementCache);
 
     static MSetElementCache *New(MDefinition *obj, MDefinition *index, MDefinition *value,
-                                 bool strict) {
-        return new MSetElementCache(obj, index, value, strict);
+                                 bool strict, bool guardHoles) {
+        return new MSetElementCache(obj, index, value, strict, guardHoles);
     }
 
     bool strict() const {
         return strict_;
     }
+    bool guardHoles() const {
+        return guardHoles_;
+    }
 
     TypePolicy *typePolicy() {
         return this;
     }
 
     bool canConsumeFloat32() const { return true; }
 };