Bug 1420172 - Make scalar replacement of arrays work in more cases (writes to COW elements, double elements). r=nbp
authorJan de Mooij <jdemooij@mozilla.com>
Sat, 06 Jan 2018 13:30:37 +0100
changeset 449891 ceb61da04fd81da9f9f835479e90359ccc50ebf7
parent 449890 a85c5795cc6f0db71e13288e849ef47b2f225270
child 449892 537f1536010aa800e0e2b7082bc43cc9878d890e
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
bugs1420172
milestone59.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 1420172 - Make scalar replacement of arrays work in more cases (writes to COW elements, double elements). r=nbp
js/src/jit-test/tests/ion/recover-cow-arrays.js
js/src/jit/Recover.cpp
js/src/jit/ScalarReplacement.cpp
--- a/js/src/jit-test/tests/ion/recover-cow-arrays.js
+++ b/js/src/jit-test/tests/ion/recover-cow-arrays.js
@@ -121,35 +121,73 @@ function array2ContentBail2(i) {
     var a = [1, 2];
     assertEq(a[0], 1);
     assertEq(a[1], 2);
     resumeHere(i);
     assertRecoveredOnBailout(a, true);
     return a.length;
 }
 
-// We don't handle COW array writes
 function arrayWrite1(i) {
     var a = [1, 2];
     a[0] = 42;
     assertEq(a[0], 42);
     assertEq(a[1], 2);
-    assertRecoveredOnBailout(a, false);
+    assertRecoveredOnBailout(a, true);
     return a.length;
 }
 
+// We don't handle length sets yet.
 function arrayWrite2(i) {
     var a = [1, 2];
     a.length = 1;
     assertEq(a[0], 1);
     assertEq(a[1], undefined);
     assertRecoveredOnBailout(a, false);
     return a.length;
 }
 
+function arrayWrite3(i) {
+    var a = [1, 2, 0];
+    if (i % 2 === 1)
+	a[0] = 2;
+    assertEq(a[0], 1 + (i % 2));
+    assertRecoveredOnBailout(a, true);
+    if (i % 2 === 1)
+	bailout();
+    assertEq(a[0], 1 + (i % 2));
+    return a.length;
+}
+
+function arrayWrite4(i) {
+    var a = [1, 2, 0];
+    for (var x = 0; x < 2; x++) {
+	if (x % 2 === 1)
+	    bailout();
+	else
+	    a[0] = a[0] + 1;
+    }
+    assertEq(a[0], 2);
+    assertEq(a[1], 2);
+    assertRecoveredOnBailout(a, true);
+    return a.length;
+}
+
+function arrayWriteDoubles(i) {
+    var a = [0, 0, 0];
+    a[0] = 3.14;
+    // MConvertElementsToDoubles is only used for loads inside a loop.
+    for (var x = 0; x < 2; x++) {
+        assertEq(a[0], 3.14);
+        assertEq(a[1], 0);
+    }
+    assertRecoveredOnBailout(a, true);
+    return a.length;
+}
+
 // Check escape analysis in case of holes.
 function arrayHole0(i) {
     var a = [1,,3];
     assertEq(a[0], 1);
     assertEq(a[1], undefined);
     assertEq(a[2], 3);
     // need to check for holes.
     assertRecoveredOnBailout(a, false);
@@ -195,16 +233,19 @@ for (var i = 0; i < 100; i++) {
     array1ContentBail0(i);
     array1ContentBail1(i);
     array2Content(i);
     array2ContentBail0(i);
     array2ContentBail1(i);
     array2ContentBail2(i);
     arrayWrite1(i);
     arrayWrite2(i);
+    arrayWrite3(i);
+    arrayWrite4(i);
+    arrayWriteDoubles(i);
     arrayHole0(i);
     arrayAlloc(i);
 }
 
 // If arr[1] is not defined, then we fallback on the prototype which instead of
 // returning undefined, returns "0".
 Object.defineProperty(Array.prototype, 1, {
   value: 100,
--- a/js/src/jit/Recover.cpp
+++ b/js/src/jit/Recover.cpp
@@ -1757,34 +1757,41 @@ RArrayState::RArrayState(CompactBufferRe
 bool
 RArrayState::recover(JSContext* cx, SnapshotIterator& iter) const
 {
     RootedValue result(cx);
     ArrayObject* object = &iter.read().toObject().as<ArrayObject>();
     uint32_t initLength = iter.read().toInt32();
 
     if (!object->denseElementsAreCopyOnWrite()) {
+        MOZ_ASSERT(object->getDenseInitializedLength() == 0,
+                   "initDenseElement call below relies on this");
         object->setDenseInitializedLength(initLength);
+
         for (size_t index = 0; index < numElements(); index++) {
             Value val = iter.read();
 
             if (index >= initLength) {
                 MOZ_ASSERT(val.isUndefined());
                 continue;
             }
 
             object->initDenseElement(index, val);
         }
     } else {
-        MOZ_ASSERT(object->getDenseInitializedLength() == numElements());
-        MOZ_ASSERT(initLength == numElements());
+        MOZ_RELEASE_ASSERT(object->getDenseInitializedLength() == numElements());
+        MOZ_RELEASE_ASSERT(initLength == numElements());
 
         for (size_t index = 0; index < numElements(); index++) {
             Value val = iter.read();
-            MOZ_RELEASE_ASSERT(object->getDenseElement(index) == val);
+            if (object->getDenseElement(index) == val)
+                continue;
+            if (!object->maybeCopyElementsForWrite(cx))
+                return false;
+            object->setDenseElement(index, val);
         }
     }
 
     result.setObject(*object);
     iter.storeInstructionResult(result);
     return true;
 }
 
--- a/js/src/jit/ScalarReplacement.cpp
+++ b/js/src/jit/ScalarReplacement.cpp
@@ -870,18 +870,20 @@ IndexOf(MDefinition* ins, int32_t* res)
         return false;
     *res = indexDefConst->toInt32();
     return true;
 }
 
 // Returns False if the elements is not escaped and if it is optimizable by
 // ScalarReplacementOfArray.
 static bool
-IsElementEscaped(MElements* def, uint32_t arraySize, bool copyOnWrite)
+IsElementEscaped(MDefinition* def, uint32_t arraySize)
 {
+    MOZ_ASSERT(def->isElements() || def->isConvertElementsToDoubles());
+
     JitSpewDef(JitSpew_Escape, "Check elements\n", def);
     JitSpewIndent spewIndent(JitSpew_Escape);
 
     for (MUseIterator i(def->usesBegin()); i != def->usesEnd(); i++) {
         // The MIRType::Elements cannot be captured in a resume point as
         // it does not represent a value allocation.
         MDefinition* access = (*i)->consumer()->toDefinition();
 
@@ -914,21 +916,16 @@ IsElementEscaped(MElements* def, uint32_
                 return true;
             }
             break;
           }
 
           case MDefinition::Opcode::StoreElement: {
             MOZ_ASSERT(access->toStoreElement()->elements() == def);
 
-            if (copyOnWrite) {
-                JitSpewDef(JitSpew_Escape, "write to COW\n", access);
-                return true;
-            }
-
             // If we need hole checks, then the array cannot be escaped
             // as the array might refer to the prototype chain to look
             // for properties, thus it might do additional side-effects
             // which are not reflected by the alias set, is we are
             // bailing on holes.
             if (access->toStoreElement()->needsHoleCheck()) {
                 JitSpewDef(JitSpew_Escape,
                            "has a store element with a hole check\n", access);
@@ -951,32 +948,35 @@ IsElementEscaped(MElements* def, uint32_
             if (access->toStoreElement()->value()->type() == MIRType::MagicHole) {
                 JitSpewDef(JitSpew_Escape, "has a store element with an magic-hole constant\n", access);
                 return true;
             }
             break;
           }
 
           case MDefinition::Opcode::SetInitializedLength:
-            if (copyOnWrite) {
-                JitSpewDef(JitSpew_Escape, "write to COW\n", access);
-                return true;
-            }
-
             MOZ_ASSERT(access->toSetInitializedLength()->elements() == def);
             break;
 
           case MDefinition::Opcode::InitializedLength:
             MOZ_ASSERT(access->toInitializedLength()->elements() == def);
             break;
 
           case MDefinition::Opcode::ArrayLength:
             MOZ_ASSERT(access->toArrayLength()->elements() == def);
             break;
 
+          case MDefinition::Opcode::ConvertElementsToDoubles:
+            MOZ_ASSERT(access->toConvertElementsToDoubles()->elements() == def);
+            if (IsElementEscaped(access, arraySize)) {
+                JitSpewDef(JitSpew_Escape, "is indirectly escaped by\n", access);
+                return true;
+            }
+            break;
+
           default:
             JitSpewDef(JitSpew_Escape, "is escaped by\n", access);
             return true;
         }
     }
     JitSpew(JitSpew_Escape, "Elements is not escaped");
     return false;
 }
@@ -988,34 +988,36 @@ IsOptimizableArrayInstruction(MInstructi
 }
 
 // Returns False if the array is not escaped and if it is optimizable by
 // ScalarReplacementOfArray.
 //
 // For the moment, this code is dumb as it only supports arrays which are not
 // changing length, with only access with known constants.
 static bool
-IsArrayEscaped(MInstruction* ins)
+IsArrayEscaped(MInstruction* ins, MInstruction* newArray)
 {
     MOZ_ASSERT(ins->type() == MIRType::Object);
-    MOZ_ASSERT(IsOptimizableArrayInstruction(ins));
+    MOZ_ASSERT(IsOptimizableArrayInstruction(ins) ||
+               ins->isMaybeCopyElementsForWrite());
+    MOZ_ASSERT(IsOptimizableArrayInstruction(newArray));
 
     JitSpewDef(JitSpew_Escape, "Check array\n", ins);
     JitSpewIndent spewIndent(JitSpew_Escape);
 
     uint32_t length;
-    if (ins->isNewArray()) {
-        if (!ins->toNewArray()->templateObject()) {
+    if (newArray->isNewArray()) {
+        if (!newArray->toNewArray()->templateObject()) {
             JitSpew(JitSpew_Escape, "No template object defined.");
             return true;
         }
 
-        length = ins->toNewArray()->length();
+        length = newArray->toNewArray()->length();
     } else {
-        length = ins->toNewArrayCopyOnWrite()->templateObject()->length();
+        length = newArray->toNewArrayCopyOnWrite()->templateObject()->length();
     }
 
     if (length >= 16) {
         JitSpew(JitSpew_Escape, "Array has too many elements");
         return true;
     }
 
     // Check if the object is escaped. If the object is not the first argument
@@ -1032,24 +1034,34 @@ IsArrayEscaped(MInstruction* ins)
             continue;
         }
 
         MDefinition* def = consumer->toDefinition();
         switch (def->op()) {
           case MDefinition::Opcode::Elements: {
             MElements *elem = def->toElements();
             MOZ_ASSERT(elem->object() == ins);
-            if (IsElementEscaped(elem, length, ins->isNewArrayCopyOnWrite())) {
+            if (IsElementEscaped(elem, length)) {
                 JitSpewDef(JitSpew_Escape, "is indirectly escaped by\n", elem);
                 return true;
             }
 
             break;
           }
 
+          case MDefinition::Opcode::MaybeCopyElementsForWrite: {
+            MMaybeCopyElementsForWrite* copied = def->toMaybeCopyElementsForWrite();
+            MOZ_ASSERT(copied->object() == ins);
+            if (IsArrayEscaped(copied, ins)) {
+                JitSpewDef(JitSpew_Escape, "is indirectly escaped by\n", copied);
+                return true;
+            }
+            break;
+          }
+
           // This instruction is a no-op used to verify that scalar replacement
           // is working as expected in jit-test.
           case MDefinition::Opcode::AssertRecoveredOnBailout:
             break;
 
           default:
             JitSpewDef(JitSpew_Escape, "is escaped by\n", def);
             return true;
@@ -1109,16 +1121,18 @@ class ArrayMemoryView : public MDefiniti
   public:
     void visitResumePoint(MResumePoint* rp);
     void visitArrayState(MArrayState* ins);
     void visitStoreElement(MStoreElement* ins);
     void visitLoadElement(MLoadElement* ins);
     void visitSetInitializedLength(MSetInitializedLength* ins);
     void visitInitializedLength(MInitializedLength* ins);
     void visitArrayLength(MArrayLength* ins);
+    void visitMaybeCopyElementsForWrite(MMaybeCopyElementsForWrite* ins);
+    void visitConvertElementsToDoubles(MConvertElementsToDoubles* ins);
 };
 
 const char* ArrayMemoryView::phaseName = "Scalar Replacement of Array";
 
 ArrayMemoryView::ArrayMemoryView(TempAllocator& alloc, MInstruction* arr)
   : alloc_(alloc),
     undefinedVal_(nullptr),
     length_(nullptr),
@@ -1401,16 +1415,57 @@ ArrayMemoryView::visitArrayLength(MArray
         arr_->block()->insertBefore(arr_, length_);
     }
     ins->replaceAllUsesWith(length_);
 
     // Remove original instruction.
     discardInstruction(ins, elements);
 }
 
+void
+ArrayMemoryView::visitMaybeCopyElementsForWrite(MMaybeCopyElementsForWrite* ins)
+{
+    MOZ_ASSERT(ins->numOperands() == 1);
+    MOZ_ASSERT(ins->type() == MIRType::Object);
+
+    // Skip guards on other objects.
+    if (ins->object() != arr_)
+        return;
+
+    // Nothing to do here: RArrayState::recover will copy the elements if
+    // needed.
+
+    // Replace the guard with the array.
+    ins->replaceAllUsesWith(arr_);
+
+    // Remove original instruction.
+    ins->block()->discard(ins);
+}
+
+void
+ArrayMemoryView::visitConvertElementsToDoubles(MConvertElementsToDoubles* ins)
+{
+    MOZ_ASSERT(ins->numOperands() == 1);
+    MOZ_ASSERT(ins->type() == MIRType::Elements);
+
+    // Skip other array objects.
+    MDefinition* elements = ins->elements();
+    if (!isArrayStateElements(elements))
+        return;
+
+    // We don't have to do anything else here: MConvertElementsToDoubles just
+    // exists to allow MLoadELement to use masm.loadDouble (without checking
+    // for int32 elements), but since we're using scalar replacement for the
+    // elements that doesn't matter.
+    ins->replaceAllUsesWith(elements);
+
+    // Remove original instruction.
+    ins->block()->discard(ins);
+}
+
 bool
 ScalarReplacement(MIRGenerator* mir, MIRGraph& graph)
 {
     EmulateStateOf<ObjectMemoryView> replaceObject(mir, graph);
     EmulateStateOf<ArrayMemoryView> replaceArray(mir, graph);
     bool addedPhi = false;
 
     for (ReversePostorderIterator block = graph.rpoBegin(); block != graph.rpoEnd(); block++) {
@@ -1423,17 +1478,17 @@ ScalarReplacement(MIRGenerator* mir, MIR
                 ObjectMemoryView view(graph.alloc(), *ins);
                 if (!replaceObject.run(view))
                     return false;
                 view.assertSuccess();
                 addedPhi = true;
                 continue;
             }
 
-            if (IsOptimizableArrayInstruction(*ins) && !IsArrayEscaped(*ins)) {
+            if (IsOptimizableArrayInstruction(*ins) && !IsArrayEscaped(*ins, *ins)) {
                 ArrayMemoryView view(graph.alloc(), *ins);
                 if (!replaceArray.run(view))
                     return false;
                 view.assertSuccess();
                 addedPhi = true;
                 continue;
             }
         }