Changeset 184747 in webkit

Timestamp:

May 21, 2015 7:39:25 PM (9 years ago)

Author:

saambarati1@gmail.com

Message:

Object allocation sinking phase should explicitly create bottom values for CreateActivation sink candidates and CreateActivation should have SymbolTable as a child node
​https://bugs.webkit.org/show_bug.cgi?id=145192

Reviewed by Filip Pizlo.

When we sink CreateActivation and generate MaterializeCreateActivation
in the object allocation sinking phase, we now explictly add PutHints for
all variables on the activation setting those variables to their default value
(undefined for Function activations and soon to be JS Empty Value for block scope activations).
This allows us to remove code that fills FTL fast activation allocations with Undefined.

This patch also adds the constant SymbolTable as an OpInfo of CreateActivation and MaterializeCreateActivation
nodes. This is in preparation for ES6 block scoping which will introduce a new
op code that gets lowered to CreateActivation.

• dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::parseBlock):

• dfg/DFGClobberize.h:

(JSC::DFG::clobberize):

• dfg/DFGNode.h:

(JSC::DFG::Node::hasCellOperand):
(JSC::DFG::Node::cellOperand):

• dfg/DFGObjectAllocationSinkingPhase.cpp:

(JSC::DFG::ObjectAllocationSinkingPhase::lowerNonReadingOperationsOnPhantomAllocations):
(JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
(JSC::DFG::ObjectAllocationSinkingPhase::createMaterialize):
(JSC::DFG::ObjectAllocationSinkingPhase::populateMaterialize):

• dfg/DFGPromotedHeapLocation.cpp:

(WTF::printInternal):

• dfg/DFGPromotedHeapLocation.h:
• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileCreateActivation):

• ftl/FTLLowerDFGToLLVM.cpp:

(JSC::FTL::LowerDFGToLLVM::compileCreateActivation):
(JSC::FTL::LowerDFGToLLVM::compileMaterializeCreateActivation):

• ftl/FTLOperations.cpp:

(JSC::FTL::operationMaterializeObjectInOSR):

• tests/stress/activation-sink-default-value.js: Added.

(bar):

• tests/stress/activation-sink-osrexit-default-value.js: Added.

(foo.set result):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGByteCodeParser.cpp (modified) (1 diff)
• dfg/DFGClobberize.h (modified) (1 diff)
• dfg/DFGNode.h (modified) (2 diffs)
• dfg/DFGObjectAllocationSinkingPhase.cpp (modified) (7 diffs)
• dfg/DFGPromotedHeapLocation.cpp (modified) (1 diff)
• dfg/DFGPromotedHeapLocation.h (modified) (1 diff)
• dfg/DFGSpeculativeJIT.cpp (modified) (1 diff)
• ftl/FTLLowerDFGToLLVM.cpp (modified) (5 diffs)
• ftl/FTLOperations.cpp (modified) (2 diffs)
• tests/stress/activation-sink-default-value.js (added)
• tests/stress/activation-sink-osrexit-default-value.js (added)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-05-21  Saam Barati  <saambarati1@gmail.com>
+
+        Object allocation sinking phase should explicitly create bottom values for CreateActivation sink candidates and CreateActivation should have SymbolTable as a child node
+        https://bugs.webkit.org/show_bug.cgi?id=145192
+
+        Reviewed by Filip Pizlo.
+
+        When we sink CreateActivation and generate MaterializeCreateActivation
+        in the object allocation sinking phase, we now explictly add PutHints for 
+        all variables on the activation setting those variables to their default value 
+        (undefined for Function activations and soon to be JS Empty Value for block scope activations). 
+        This allows us to remove code that fills FTL fast activation allocations with Undefined.
+
+        This patch also adds the constant SymbolTable as an OpInfo of CreateActivation and MaterializeCreateActivation
+        nodes. This is in preparation for ES6 block scoping which will introduce a new 
+        op code that gets lowered to CreateActivation.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::hasCellOperand):
+        (JSC::DFG::Node::cellOperand):
+        * dfg/DFGObjectAllocationSinkingPhase.cpp:
+        (JSC::DFG::ObjectAllocationSinkingPhase::lowerNonReadingOperationsOnPhantomAllocations):
+        (JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
+        (JSC::DFG::ObjectAllocationSinkingPhase::createMaterialize):
+        (JSC::DFG::ObjectAllocationSinkingPhase::populateMaterialize):
+        * dfg/DFGPromotedHeapLocation.cpp:
+        (WTF::printInternal):
+        * dfg/DFGPromotedHeapLocation.h:
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileCreateActivation):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::compileCreateActivation):
+        (JSC::FTL::LowerDFGToLLVM::compileMaterializeCreateActivation):
+        * ftl/FTLOperations.cpp:
+        (JSC::FTL::operationMaterializeObjectInOSR):
+        * tests/stress/activation-sink-default-value.js: Added.
+        (bar):
+        * tests/stress/activation-sink-osrexit-default-value.js: Added.
+        (foo.set result):
+
 2015-05-21  Per Arne Vollan  <peavo@outlook.com>
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -3734 +3734 @@
             
         case op_create_lexical_environment: {
-            Node* lexicalEnvironment = addToGraph(CreateActivation, get(VirtualRegister(currentInstruction[2].u.operand)));
+            FrozenValue* symbolTable = m_graph.freezeStrong(m_graph.symbolTableFor(currentNodeOrigin().semantic));
+            Node* lexicalEnvironment = addToGraph(CreateActivation, OpInfo(symbolTable), get(VirtualRegister(currentInstruction[2].u.operand)));
             set(VirtualRegister(currentInstruction[1].u.operand), lexicalEnvironment);
             set(VirtualRegister(currentInstruction[2].u.operand), lexicalEnvironment);

trunk/Source/JavaScriptCore/dfg/DFGClobberize.h

@@ -318 +318 @@
 
     case CreateActivation: {
-        SymbolTable* table = graph.symbolTableFor(node->origin.semantic);
+        SymbolTable* table = node->castOperand<SymbolTable*>();
         if (table->singletonScope()->isStillValid())
             write(Watchpoint_fire);

trunk/Source/JavaScriptCore/dfg/DFGNode.h

@@ -1283 +1283 @@
         case NativeCall:
         case NewFunction:
+        case CreateActivation:
+        case MaterializeCreateActivation:
             return true;
         default:
@@ -1292 +1294 @@
     {
         ASSERT(hasCellOperand());
-        return reinterpret_cast<FrozenValue*>(m_opInfo);
+        switch (op()) {
+        case MaterializeCreateActivation:
+            return reinterpret_cast<FrozenValue*>(m_opInfo2);
+        default:
+            return reinterpret_cast<FrozenValue*>(m_opInfo);
+        }
+        RELEASE_ASSERT_NOT_REACHED();
     }
     

trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -587 +587 @@
                             PromotedHeapLocation(ActivationScopePLoc, node).createHint(
                                 m_graph, node->origin, node->child1().node()));
+                        Node* symbolTableNode = m_insertionSet.insertConstant(
+                            nodeIndex + 1, node->origin, node->cellOperand());
+                        m_insertionSet.insert(
+                            nodeIndex + 1,
+                            PromotedHeapLocation(ActivationSymbolTablePLoc, node).createHint(
+                                m_graph, node->origin, symbolTableNode));
+
+                        {
+                            SymbolTable* symbolTable = node->castOperand<SymbolTable*>();
+                            Node* undefined = m_insertionSet.insertConstant(
+                                nodeIndex + 1, node->origin, jsUndefined());
+                            ConcurrentJITLocker locker(symbolTable->m_lock);
+                            for (auto iter = symbolTable->begin(locker), end = symbolTable->end(locker); iter != end; ++iter) {
+                                m_insertionSet.insert(
+                                    nodeIndex + 1,
+                                    PromotedHeapLocation(
+                                        ClosureVarPLoc, node, iter->value.scopeOffset().offset()).createHint(
+                                        m_graph, node->origin, undefined));
+                            }
+                        }
+
                         node->convertToPhantomCreateActivation();
                     }
@@ -598 +619 @@
                             PromotedHeapLocation(ActivationScopePLoc, node).createHint(
                                 m_graph, node->origin, m_graph.varArgChild(node, 0).node()));
+                        Node* symbolTableNode = m_insertionSet.insertConstant(
+                            nodeIndex + 1, node->origin, node->cellOperand());
+                        m_insertionSet.insert(
+                            nodeIndex + 1,
+                            PromotedHeapLocation(ActivationSymbolTablePLoc, node).createHint(
+                                m_graph, node->origin, symbolTableNode));
                         ObjectMaterializationData& data = node->objectMaterializationData();
                         for (unsigned i = 0; i < data.m_properties.size(); ++i) {
@@ -830 +857 @@
 
         case CreateActivation:
-            if (!m_graph.symbolTableFor(node->origin.semantic)->singletonScope()->isStillValid())
+            if (!node->castOperand<SymbolTable*>()->singletonScope()->isStillValid())
                 sinkCandidate();
             escape(node->child1().node());
@@ -933 +960 @@
         case MaterializeCreateActivation: {
             ObjectMaterializationData* data = m_graph.m_objectMaterializationData.add();
-
+            FrozenValue* symbolTable = escapee->cellOperand();
             result = m_graph.addNode(
                 escapee->prediction(), Node::VarArg, MaterializeCreateActivation,
@@ -939 +966 @@
                     escapee->origin.semantic,
                     where->origin.forExit),
-                OpInfo(data), OpInfo(), 0, 0);
+                OpInfo(data), OpInfo(symbolTable), 0, 0);
             break;
         }
@@ -1010 +1037 @@
 
             PromotedHeapLocation scope(ActivationScopePLoc, escapee);
+            PromotedHeapLocation symbolTable(ActivationSymbolTablePLoc, escapee);
             ASSERT(locations.contains(scope));
 
@@ -1018 +1046 @@
                 case ActivationScopePLoc: {
                     ASSERT(locations[i] == scope);
+                    break;
+                }
+
+                case ActivationSymbolTablePLoc: {
+                    ASSERT(locations[i] == symbolTable);
                     break;
                 }

trunk/Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.cpp

@@ -67 +67 @@
         out.print("StructurePLoc");
         return;
+
+    case ActivationSymbolTablePLoc:
+        out.print("ActivationSymbolTablePLoc");
+        return;
         
     case NamedPropertyPLoc:

trunk/Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h

@@ -38 +38 @@
     
     StructurePLoc,
+    ActivationSymbolTablePLoc,
     NamedPropertyPLoc,
     ArgumentPLoc,

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -4522 +4522 @@
 void SpeculativeJIT::compileCreateActivation(Node* node)
 {
-    SymbolTable* table = m_jit.graph().symbolTableFor(node->origin.semantic);
+    SymbolTable* table = node->castOperand<SymbolTable*>();
     Structure* structure = m_jit.graph().globalObjectFor(
         node->origin.semantic)->activationStructure();

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

@@ -2948 +2948 @@
     {
         LValue scope = lowCell(m_node->child1());
-        SymbolTable* table = m_graph.symbolTableFor(m_node->origin.semantic);
+        SymbolTable* table = m_node->castOperand<SymbolTable*>();
         Structure* structure = m_graph.globalObjectFor(m_node->origin.semantic)->activationStructure();
         
@@ -5265 +5265 @@
 
         LValue scope = lowCell(m_graph.varArgChild(m_node, 0));
-        SymbolTable* table = m_graph.symbolTableFor(m_node->origin.semantic);
+        SymbolTable* table = m_node->castOperand<SymbolTable*>();
         Structure* structure = m_graph.globalObjectFor(m_node->origin.semantic)->activationStructure();
 
@@ -5279 +5279 @@
         m_out.storePtr(weakPointer(table), fastObject, m_heaps.JSSymbolTableObject_symbolTable);
 
-        for (unsigned i = 0; i < table->scopeSize(); ++i) {
-            m_out.store64(
-                m_out.constInt64(JSValue::encode(jsUndefined())),
-                fastObject, m_heaps.JSEnvironmentRecord_variables[i]);
-        }
 
         ValueFromBlock fastResult = m_out.anchor(fastObject);
@@ -5297 +5292 @@
         m_out.appendTo(continuation, lastNext);
         LValue activation = m_out.phi(m_out.intPtr, fastResult, slowResult);
+        RELEASE_ASSERT(data.m_properties.size() == table->scopeSize());
         for (unsigned i = 0; i < data.m_properties.size(); ++i) {
             m_out.store64(values[i],
@@ -5302 +5298 @@
                 m_heaps.JSEnvironmentRecord_variables[data.m_properties[i].m_identifierNumber]);
         }
+
+        if (validationEnabled()) {
+            // Validate to make sure every slot in the scope has one value.
+            ConcurrentJITLocker locker(table->m_lock);
+            for (auto iter = table->begin(locker), end = table->end(locker); iter != end; ++iter) {
+                bool found = false;
+                for (unsigned i = 0; i < data.m_properties.size(); ++i) {
+                    if (iter->value.scopeOffset().offset() == data.m_properties[i].m_identifierNumber) {
+                        found = true;
+                        break;
+                    }
+                }
+                ASSERT(found);
+            }
+        }
+
         setJSValue(activation);
     }

trunk/Source/JavaScriptCore/ftl/FTLOperations.cpp

@@ -116 +116 @@
         // Figure out where the scope is
         JSScope* scope = nullptr;
-        for (unsigned i = materialization->properties().size(); i--;) {
-            const ExitPropertyValue& property = materialization->properties()[i];
-            if (property.location() != PromotedLocationDescriptor(ActivationScopePLoc))
-                continue;
-            scope = jsCast<JSScope*>(JSValue::decode(values[i]));
+        SymbolTable* table = nullptr;
+        for (unsigned i = materialization->properties().size(); i--;) {
+            const ExitPropertyValue& property = materialization->properties()[i];
+            if (property.location() == PromotedLocationDescriptor(ActivationScopePLoc))
+                scope = jsCast<JSScope*>(JSValue::decode(values[i]));
+            else if (property.location() == PromotedLocationDescriptor(ActivationSymbolTablePLoc))
+                table = jsCast<SymbolTable*>(JSValue::decode(values[i]));
         }
         RELEASE_ASSERT(scope);
+        RELEASE_ASSERT(table);
 
         CodeBlock* codeBlock = baselineCodeBlockForOriginAndBaselineCodeBlock(
             materialization->origin(), exec->codeBlock());
-        SymbolTable* table = codeBlock->symbolTable();
         Structure* structure = codeBlock->globalObject()->activationStructure();
 
         JSLexicalEnvironment* result = JSLexicalEnvironment::create(vm, structure, scope, table);
 
+        RELEASE_ASSERT(materialization->properties().size() - 2 == table->scopeSize());
         // Figure out what to populate the activation with
         for (unsigned i = materialization->properties().size(); i--;) {
@@ -138 +141 @@
 
             result->variableAt(ScopeOffset(property.location().info())).set(exec->vm(), result, JSValue::decode(values[i]));
+        }
+
+        if (validationEnabled()) {
+            // Validate to make sure every slot in the scope has one value.
+            ConcurrentJITLocker locker(table->m_lock);
+            for (auto iter = table->begin(locker), end = table->end(locker); iter != end; ++iter) {
+                bool found = false;
+                for (unsigned i = materialization->properties().size(); i--;) {
+                    const ExitPropertyValue& property = materialization->properties()[i];
+                    if (property.location().kind() != ClosureVarPLoc)
+                        continue;
+                    if (ScopeOffset(property.location().info()) == iter->value.scopeOffset()) {
+                        found = true;
+                        break;
+                    }
+                }
+                ASSERT(found);
+            }
+            unsigned numberOfClosureVarPloc = 0;
+            for (unsigned i = materialization->properties().size(); i--;) {
+                const ExitPropertyValue& property = materialization->properties()[i];
+                if (property.location().kind() == ClosureVarPLoc)
+                    numberOfClosureVarPloc++;
+            }
+            ASSERT(numberOfClosureVarPloc == table->scopeSize());
         }