GetArrayLength should be "blessed" during Fixup phase in the DFG
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 May 2020 22:01:50 +0000 (22:01 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 May 2020 22:01:50 +0000 (22:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=211540

Reviewed by Saam Barati.

JSTests:

* stress/get-array-length-node-should-be-blessed-in-fixup.js: Added.
(foo):

Source/JavaScriptCore:

If we got an ArrayMode during bytecode parsing for-of that expects
to be configured during Fixup, then right now we will crash on
GetArrayLength. This fixes GetArrayLength to properly call
blessArrayOperation and fixes clobberize to know that
GetArrayLength could have a ForceExit ArrayMode briefly before
being cleaned up.

When blessing GetArrayLength we can now produce CheckArrays that
have an AnyTypedArray ArrayMode::Type. So this patch expands
CheckArray to properly handle that. To help with this we expand
branchIfType to have a starting JSType and an optional ending
JSType. Additionally, to prevent extra checks AI has been taught
to fold more ArrayModes so we should almost always avoid new
runtime checks.

Lastly, make sure that Undecided Arrays don't fall back to generic
because GetArrayLength can't be converted to...
GetArrayLenth. Also, GetArrayLength would previously pass it's own
speculation for the speculation of the index, which logically
doesn't make sense. So this patch adds a new constant, which is
SpecInt32Only, that can be passed if a DFG node doesn't have an
index.

* assembler/testmasm.cpp:
(JSC::testBranchIfType):
(JSC::testBranchIfNotType):
(JSC::run):
* dfg/DFGArrayMode.cpp:
(JSC::DFG::canBecomeGetArrayLength):
* dfg/DFGArrayMode.h:
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::blessArrayOperation):
(JSC::DFG::FixupPhase::attemptToMakeGetArrayLength):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::checkArray):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::isArrayTypeForCheckArray):
* jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::branchIfType):
(JSC::AssemblyHelpers::branchIfNotType):
* runtime/JSType.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@261712 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
JSTests/ChangeLog
JSTests/stress/get-array-length-node-should-be-blessed-in-fixup.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/testmasm.cpp
Source/JavaScriptCore/dfg/DFGArrayMode.cpp
Source/JavaScriptCore/dfg/DFGArrayMode.h
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/jit/AssemblyHelpers.h
Source/JavaScriptCore/runtime/JSType.h

index 910bf46..9f9e4ca 100644 (file)
@@ -1,3 +1,13 @@
+2020-05-14  Keith Miller  <keith_miller@apple.com>
+
+        GetArrayLength should be "blessed" during Fixup phase in the DFG
+        https://bugs.webkit.org/show_bug.cgi?id=211540
+
+        Reviewed by Saam Barati.
+
+        * stress/get-array-length-node-should-be-blessed-in-fixup.js: Added.
+        (foo):
+
 2020-05-13  Keith Miller  <keith_miller@apple.com>
 
         iteration bytecodes need to handle osr exiting from inlined getter frames
diff --git a/JSTests/stress/get-array-length-node-should-be-blessed-in-fixup.js b/JSTests/stress/get-array-length-node-should-be-blessed-in-fixup.js
new file mode 100644 (file)
index 0000000..bc98d5a
--- /dev/null
@@ -0,0 +1,8 @@
+function foo() {
+  for (let i=0; i<10000; i++) {}
+  for (const q of Array.prototype) {}
+}
+
+for (let i=0; i<1000; i++) {
+  foo();
+}
index ecb4112..6fe076a 100644 (file)
@@ -1,3 +1,55 @@
+2020-05-14  Keith Miller  <keith_miller@apple.com>
+
+        GetArrayLength should be "blessed" during Fixup phase in the DFG
+        https://bugs.webkit.org/show_bug.cgi?id=211540
+
+        Reviewed by Saam Barati.
+
+        If we got an ArrayMode during bytecode parsing for-of that expects
+        to be configured during Fixup, then right now we will crash on
+        GetArrayLength. This fixes GetArrayLength to properly call
+        blessArrayOperation and fixes clobberize to know that
+        GetArrayLength could have a ForceExit ArrayMode briefly before
+        being cleaned up.
+
+        When blessing GetArrayLength we can now produce CheckArrays that
+        have an AnyTypedArray ArrayMode::Type. So this patch expands
+        CheckArray to properly handle that. To help with this we expand
+        branchIfType to have a starting JSType and an optional ending
+        JSType. Additionally, to prevent extra checks AI has been taught
+        to fold more ArrayModes so we should almost always avoid new
+        runtime checks.
+
+        Lastly, make sure that Undecided Arrays don't fall back to generic
+        because GetArrayLength can't be converted to...
+        GetArrayLenth. Also, GetArrayLength would previously pass it's own
+        speculation for the speculation of the index, which logically
+        doesn't make sense. So this patch adds a new constant, which is
+        SpecInt32Only, that can be passed if a DFG node doesn't have an
+        index.
+
+        * assembler/testmasm.cpp:
+        (JSC::testBranchIfType):
+        (JSC::testBranchIfNotType):
+        (JSC::run):
+        * dfg/DFGArrayMode.cpp:
+        (JSC::DFG::canBecomeGetArrayLength):
+        * dfg/DFGArrayMode.h:
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        (JSC::DFG::FixupPhase::blessArrayOperation):
+        (JSC::DFG::FixupPhase::attemptToMakeGetArrayLength):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::checkArray):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::isArrayTypeForCheckArray):
+        * jit/AssemblyHelpers.h:
+        (JSC::AssemblyHelpers::branchIfType):
+        (JSC::AssemblyHelpers::branchIfNotType):
+        * runtime/JSType.h:
+
 2020-05-13  Keith Miller  <keith_miller@apple.com>
 
         iteration bytecodes need to handle osr exiting from inlined getter frames
index 4c9f1de..0f263ea 100644 (file)
@@ -2161,6 +2161,74 @@ static void testCagePreservesPACFailureBit()
 #endif
 }
 
+static void testBranchIfType()
+{
+    using JSC::JSType;
+    struct CellLike {
+        uint32_t structureID;
+        uint8_t indexingType;
+        JSType type;
+    };
+    CHECK_EQ(JSCell::typeInfoTypeOffset(), OBJECT_OFFSETOF(CellLike, type));
+
+    auto isType = compile([] (CCallHelpers& jit) {
+        emitFunctionPrologue(jit);
+        auto isType = jit.branchIfType(GPRInfo::argumentGPR0, JSType(FirstTypedArrayType), JSType(LastTypedArrayTypeExcludingDataView));
+        jit.move(CCallHelpers::TrustedImm32(false), GPRInfo::returnValueGPR);
+        auto done = jit.jump();
+        isType.link(&jit);
+        jit.move(CCallHelpers::TrustedImm32(true), GPRInfo::returnValueGPR);
+        done.link(&jit);
+        emitFunctionEpilogue(jit);
+        jit.ret();
+    });
+
+    CellLike cell;
+    for (unsigned i = JSC::FirstTypedArrayType; i <= JSC::LastTypedArrayTypeExcludingDataView; ++i) {
+        cell.type = JSType(i);
+        CHECK_EQ(invoke<bool>(isType, &cell), true);
+    }
+
+    cell.type = JSType(LastTypedArrayType);
+    CHECK_EQ(invoke<bool>(isType, &cell), false);
+    cell.type = JSType(FirstTypedArrayType - 1);
+    CHECK_EQ(invoke<bool>(isType, &cell), false);
+}
+
+static void testBranchIfNotType()
+{
+    using JSC::JSType;
+    struct CellLike {
+        uint32_t structureID;
+        uint8_t indexingType;
+        JSType type;
+    };
+    CHECK_EQ(JSCell::typeInfoTypeOffset(), OBJECT_OFFSETOF(CellLike, type));
+
+    auto isNotType = compile([] (CCallHelpers& jit) {
+        emitFunctionPrologue(jit);
+        auto isNotType = jit.branchIfNotType(GPRInfo::argumentGPR0, JSType(FirstTypedArrayType), JSType(LastTypedArrayTypeExcludingDataView));
+        jit.move(CCallHelpers::TrustedImm32(false), GPRInfo::returnValueGPR);
+        auto done = jit.jump();
+        isNotType.link(&jit);
+        jit.move(CCallHelpers::TrustedImm32(true), GPRInfo::returnValueGPR);
+        done.link(&jit);
+        emitFunctionEpilogue(jit);
+        jit.ret();
+    });
+
+    CellLike cell;
+    for (unsigned i = JSC::FirstTypedArrayType; i <= JSC::LastTypedArrayTypeExcludingDataView; ++i) {
+        cell.type = JSType(i);
+        CHECK_EQ(invoke<bool>(isNotType, &cell), false);
+    }
+
+    cell.type = JSType(LastTypedArrayType);
+    CHECK_EQ(invoke<bool>(isNotType, &cell), true);
+    cell.type = JSType(FirstTypedArrayType - 1);
+    CHECK_EQ(invoke<bool>(isNotType, &cell), true);
+}
+
 #define RUN(test) do {                          \
         if (!shouldRun(#test))                  \
             break;                              \
@@ -2283,6 +2351,9 @@ void run(const char* filter)
 
     RUN(testCagePreservesPACFailureBit());
 
+    RUN(testBranchIfType());
+    RUN(testBranchIfNotType());
+
     RUN(testOrImmMem());
 
     if (tasks.isEmpty())
index f32245f..c701989 100644 (file)
@@ -185,6 +185,8 @@ ArrayMode ArrayMode::fromObserved(const ConcurrentJSLocker& locker, ArrayProfile
 
 static bool canBecomeGetArrayLength(Graph& graph, Node* node)
 {
+    if (node->op() == GetArrayLength)
+        return true;
     if (node->op() != GetById)
         return false;
     auto uid = node->cacheableIdentifier().uid();
index fdb71e7..c304655 100644 (file)
@@ -237,6 +237,7 @@ public:
         return ArrayMode(type, arrayClass(), speculation(), conversion, action());
     }
     
+    static constexpr SpeculatedType unusedIndexSpeculatedType = SpecInt32Only;
     ArrayMode refine(Graph&, Node*, SpeculatedType base, SpeculatedType index, SpeculatedType value = SpecNone) const;
     
     bool alreadyChecked(Graph&, Node*, const AbstractValue&) const;
index e28d70a..0f6acd0 100644 (file)
@@ -1353,6 +1353,11 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             def(HeapLocation(ArrayLengthLoc, MiscFields, node->child1()), LazyNode(node));
             return;
 
+        case Array::ForceExit: {
+            write(SideState);
+            return;
+        }
+
         default:
             ASSERT(mode.isSomeTypedArrayView());
             read(MiscFields);
index f4db60a..8ae4ff8 100644 (file)
@@ -2021,6 +2021,14 @@ private:
         }
 
         case GetArrayLength: {
+            ArrayMode arrayMode = node->arrayMode().refine(m_graph, node, node->child1()->prediction(), ArrayMode::unusedIndexSpeculatedType);
+            // We don't know how to handle generic and we only emit this in the Parser when we have checked the value is an Array/TypedArray.
+            if (arrayMode.type() == Array::Generic)
+                arrayMode = arrayMode.withType(Array::ForceExit);
+            ASSERT(arrayMode.isSpecific() || arrayMode.type() == Array::ForceExit);
+            node->setArrayMode(arrayMode);
+            blessArrayOperation(node->child1(), Edge(), node->child2(), lengthNeedsStorage);
+
             fixEdge<KnownCellUse>(node->child1());
             break;
         }
@@ -3519,7 +3527,7 @@ private:
         }
     }
     
-    void blessArrayOperation(Edge base, Edge index, Edge& storageChild)
+    void blessArrayOperation(Edge base, Edge index, Edge& storageChild, bool (*storageCheck)(const ArrayMode&) = canCSEStorage)
     {
         Node* node = m_currentNode;
         
@@ -3539,7 +3547,7 @@ private:
             return;
             
         default: {
-            Node* storage = checkArray(node->arrayMode(), node->origin, base.node(), index.node());
+            Node* storage = checkArray(node->arrayMode(), node->origin, base.node(), index.node(), storageCheck);
             if (!storage)
                 return;
             
@@ -3828,7 +3836,7 @@ private:
         }
             
         arrayMode = arrayMode.refine(
-            m_graph, node, node->child1()->prediction(), node->prediction());
+            m_graph, node, node->child1()->prediction(), ArrayMode::unusedIndexSpeculatedType);
             
         if (arrayMode.type() == Array::Generic) {
             // Check if the input is something that we can't get array length for, but for which we
index ba45f0e..d279152 100644 (file)
@@ -832,13 +832,14 @@ JITCompiler::JumpList SpeculativeJIT::jumpSlowForUnwantedArrayMode(GPRReg tempGP
 
 void SpeculativeJIT::checkArray(Node* node)
 {
-    ASSERT(node->arrayMode().isSpecific());
-    ASSERT(!node->arrayMode().doesConversion());
+    ArrayMode arrayMode = node->arrayMode();
+    ASSERT(arrayMode.isSpecific());
+    ASSERT(!arrayMode.doesConversion());
     
     SpeculateCellOperand base(this, node->child1());
     GPRReg baseReg = base.gpr();
     
-    if (node->arrayMode().alreadyChecked(m_jit.graph(), node, m_state.forNode(node->child1()))) {
+    if (arrayMode.alreadyChecked(m_jit.graph(), node, m_state.forNode(node->child1()))) {
         // We can purge Empty check completely in this case of CheckArrayOrEmpty since CellUse only accepts SpecCell | SpecEmpty.
         ASSERT(typeFilterFor(node->child1().useKind()) & SpecEmpty);
         noResult(m_currentNode);
@@ -847,7 +848,7 @@ void SpeculativeJIT::checkArray(Node* node)
 
     Optional<GPRTemporary> temp;
     Optional<GPRReg> tempGPR;
-    switch (node->arrayMode().type()) {
+    switch (arrayMode.type()) {
     case Array::Int32:
     case Array::Double:
     case Array::Contiguous:
@@ -871,8 +872,7 @@ void SpeculativeJIT::checkArray(Node* node)
     }
 #endif
 
-    switch (node->arrayMode().type()) {
-    case Array::AnyTypedArray:
+    switch (arrayMode.type()) {
     case Array::String:
         RELEASE_ASSERT_NOT_REACHED(); // Should have been a Phantom(String:)
         return;
@@ -885,7 +885,7 @@ void SpeculativeJIT::checkArray(Node* node)
         m_jit.load8(MacroAssembler::Address(baseReg, JSCell::indexingTypeAndMiscOffset()), tempGPR.value());
         speculationCheck(
             BadIndexingType, JSValueSource::unboxedCell(baseReg), nullptr,
-            jumpSlowForUnwantedArrayMode(tempGPR.value(), node->arrayMode()));
+            jumpSlowForUnwantedArrayMode(tempGPR.value(), arrayMode));
         break;
     }
     case Array::DirectArguments:
@@ -894,12 +894,16 @@ void SpeculativeJIT::checkArray(Node* node)
     case Array::ScopedArguments:
         speculateCellTypeWithoutTypeFiltering(node->child1(), baseReg, ScopedArgumentsType);
         break;
-    default:
-        speculateCellTypeWithoutTypeFiltering(
-            node->child1(), baseReg,
-            typeForTypedArrayType(node->arrayMode().typedArrayType()));
+    default: {
+        DFG_ASSERT(m_graph, node, arrayMode.isSomeTypedArrayView());
+
+        if (arrayMode.type() == Array::AnyTypedArray)
+            speculationCheck(BadType, JSValueSource::unboxedCell(baseReg), nullptr, m_jit.branchIfNotType(baseReg, JSType(FirstTypedArrayType), JSType(LastTypedArrayTypeExcludingDataView)));
+        else
+            speculateCellTypeWithoutTypeFiltering(node->child1(), baseReg, typeForTypedArrayType(arrayMode.typedArrayType()));
         break;
     }
+    }
 
     if (isEmpty.isSet())
         isEmpty.link(&m_jit);
index 04ab8f9..f6ad799 100644 (file)
@@ -17959,11 +17959,15 @@ private:
                 m_out.load8ZeroExt32(cell, m_heaps.JSCell_typeInfoType),
                 m_out.constInt32(ScopedArgumentsType));
             
-        default:
+        default: {
+            DFG_ASSERT(m_graph, m_node, arrayMode.isSomeTypedArrayView());
+            if (arrayMode.type() == Array::AnyTypedArray)
+                return isTypedArrayView(cell);
             return m_out.equal(
                 m_out.load8ZeroExt32(cell, m_heaps.JSCell_typeInfoType),
                 m_out.constInt32(typeForTypedArrayType(arrayMode.typedArrayType())));
         }
+        }
     }
     
     LValue isFunction(LValue cell, SpeculatedType type = SpecFullTop)
index f9d5774..106414c 100644 (file)
@@ -980,14 +980,31 @@ public:
             Below, Address(cellGPR, JSCell::typeInfoTypeOffset()), TrustedImm32(ObjectType));
     }
     
-    Jump branchIfType(GPRReg cellGPR, JSType type)
-    {
-        return branch8(Equal, Address(cellGPR, JSCell::typeInfoTypeOffset()), TrustedImm32(type));
+    // Note that first and last are inclusive.
+    Jump branchIfType(GPRReg cellGPR, JSType first, Optional<JSType> last = WTF::nullopt)
+    {
+        if (last && *last != first) {
+            ASSERT(*last > first);
+            GPRReg scratch = scratchRegister();
+            load8(Address(cellGPR, JSCell::typeInfoTypeOffset()), scratch);
+            sub32(TrustedImm32(first), scratch);
+            return branch32(BelowOrEqual, scratch, TrustedImm32(*last - first));
+        }
+
+        return branch8(Equal, Address(cellGPR, JSCell::typeInfoTypeOffset()), TrustedImm32(first));
     }
-    
-    Jump branchIfNotType(GPRReg cellGPR, JSType type)
+
+    Jump branchIfNotType(GPRReg cellGPR, JSType first, Optional<JSType> last = WTF::nullopt)
     {
-        return branch8(NotEqual, Address(cellGPR, JSCell::typeInfoTypeOffset()), TrustedImm32(type));
+        if (last && *last != first) {
+            ASSERT(*last > first);
+            GPRReg scratch = scratchRegister();
+            load8(Address(cellGPR, JSCell::typeInfoTypeOffset()), scratch);
+            sub32(TrustedImm32(first), scratch);
+            return branch32(Above, scratch, TrustedImm32(*last - first));
+        }
+
+        return branch8(NotEqual, Address(cellGPR, JSCell::typeInfoTypeOffset()), TrustedImm32(first));
     }
 
     // FIXME: rename these to make it clear that they require their input to be a cell.
index b2e1629..455b605 100644 (file)
@@ -132,6 +132,7 @@ enum JSType : uint8_t {
 
 static constexpr uint32_t FirstTypedArrayType = Int8ArrayType;
 static constexpr uint32_t LastTypedArrayType = DataViewType;
+static constexpr uint32_t LastTypedArrayTypeExcludingDataView = LastTypedArrayType - 1;
 
 // LastObjectType should be MaxJSType (not LastJSCObjectType) since embedders can add their extended object types after the enums listed in JSType.
 static constexpr uint32_t FirstObjectType = ObjectType;