Attachment 253566 Details for Bug 145192 – patch

[patch] patch

work-backup.diff (text/plain), 19.76 KB, created by Saam Barati on 2015-05-21 18:12:24 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2015-05-21 18:12:24 PDT

Size: 19.76 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 184744)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,47 @@
>+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 NOBODY (OOPS!).
>+
>+        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>
> 
>         MSVC internal compiler error when compiling TemplateRegistryKey class.
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -3731,7 +3731,8 @@ bool ByteCodeParser::parseBlock(unsigned
>         }
>             
>         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);
>             NEXT_OPCODE(op_create_lexical_environment);
>Index: Source/JavaScriptCore/dfg/DFGClobberize.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGClobberize.h	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGClobberize.h	(working copy)
>@@ -317,7 +317,7 @@ void clobberize(Graph& graph, Node* node
>         return;
> 
>     case CreateActivation: {
>-        SymbolTable* table = graph.symbolTableFor(node->origin.semantic);
>+        SymbolTable* table = node->castOperand<SymbolTable*>();
>         if (table->singletonScope()->isStillValid())
>             write(Watchpoint_fire);
>         read(HeapObjectCount);
>Index: Source/JavaScriptCore/dfg/DFGNode.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGNode.h	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGNode.h	(working copy)
>@@ -1282,6 +1282,8 @@ struct Node {
>         case NativeConstruct:
>         case NativeCall:
>         case NewFunction:
>+        case CreateActivation:
>+        case MaterializeCreateActivation:
>             return true;
>         default:
>             return false;
>@@ -1291,7 +1293,13 @@ struct Node {
>     FrozenValue* cellOperand()
>     {
>         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();
>     }
>     
>     template<typename T>
>Index: Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp	(working copy)
>@@ -586,6 +586,27 @@ private:
>                             nodeIndex + 1,
>                             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();
>                     }
>                     break;
>@@ -597,6 +618,12 @@ private:
>                             nodeIndex + 1,
>                             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) {
>                             unsigned identifierNumber = data.m_properties[i].m_identifierNumber;
>@@ -829,7 +856,7 @@ private:
>             break;
> 
>         case CreateActivation:
>-            if (!m_graph.symbolTableFor(node->origin.semantic)->singletonScope()->isStillValid())
>+            if (!node->castOperand<SymbolTable*>()->singletonScope()->isStillValid())
>                 sinkCandidate();
>             escape(node->child1().node());
>             break;
>@@ -932,13 +959,13 @@ private:
>         case CreateActivation:
>         case MaterializeCreateActivation: {
>             ObjectMaterializationData* data = m_graph.m_objectMaterializationData.add();
>-
>+            FrozenValue* symbolTable = escapee->cellOperand();
>             result = m_graph.addNode(
>                 escapee->prediction(), Node::VarArg, MaterializeCreateActivation,
>                 NodeOrigin(
>                     escapee->origin.semantic,
>                     where->origin.forExit),
>-                OpInfo(data), OpInfo(), 0, 0);
>+                OpInfo(data), OpInfo(symbolTable), 0, 0);
>             break;
>         }
> 
>@@ -1009,6 +1036,7 @@ private:
>             Vector<PromotedHeapLocation> locations = m_locationsForAllocation.get(escapee);
> 
>             PromotedHeapLocation scope(ActivationScopePLoc, escapee);
>+            PromotedHeapLocation symbolTable(ActivationSymbolTablePLoc, escapee);
>             ASSERT(locations.contains(scope));
> 
>             m_graph.m_varArgChildren.append(Edge(resolve(block, scope), KnownCellUse));
>@@ -1020,6 +1048,11 @@ private:
>                     break;
>                 }
> 
>+                case ActivationSymbolTablePLoc: {
>+                    ASSERT(locations[i] == symbolTable);
>+                    break;
>+                }
>+
>                 case ClosureVarPLoc: {
>                     Node* value = resolve(block, locations[i]);
>                     if (value->op() == BottomValue)
>Index: Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.cpp	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.cpp	(working copy)
>@@ -66,6 +66,10 @@ void printInternal(PrintStream& out, Pro
>     case StructurePLoc:
>         out.print("StructurePLoc");
>         return;
>+
>+    case ActivationSymbolTablePLoc:
>+        out.print("ActivationSymbolTablePLoc");
>+        return;
>         
>     case NamedPropertyPLoc:
>         out.print("NamedPropertyPLoc");
>Index: Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h	(working copy)
>@@ -37,6 +37,7 @@ enum PromotedLocationKind {
>     InvalidPromotedLocationKind,
>     
>     StructurePLoc,
>+    ActivationSymbolTablePLoc,
>     NamedPropertyPLoc,
>     ArgumentPLoc,
>     ArgumentCountPLoc,
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(revision 184586)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(working copy)
>@@ -4521,7 +4521,7 @@ void SpeculativeJIT::compileForwardVarar
> 
> 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();
>         
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(revision 184586)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(working copy)
>@@ -2947,7 +2947,7 @@ private:
>     void compileCreateActivation()
>     {
>         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();
>         
>         if (table->singletonScope()->isStillValid()) {
>@@ -5264,7 +5264,7 @@ private:
>             values.append(lowJSValue(m_graph.varArgChild(m_node, 1 + i)));
> 
>         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();
> 
>         LBasicBlock slowPath = FTL_NEW_BLOCK(m_out, ("MaterializeCreateActivation slow path"));
>@@ -5278,11 +5278,6 @@ private:
>         m_out.storePtr(scope, fastObject, m_heaps.JSScope_next);
>         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);
>         m_out.jump(continuation);
>@@ -5296,11 +5291,28 @@ private:
> 
>         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],
>                 activation,
>                 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);
>     }
> 
>Index: Source/JavaScriptCore/ftl/FTLOperations.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLOperations.cpp	(revision 184586)
>+++ Source/JavaScriptCore/ftl/FTLOperations.cpp	(working copy)
>@@ -115,21 +115,24 @@ extern "C" JSCell* JIT_OPERATION operati
>     case PhantomCreateActivation: {
>         // Figure out where the scope is
>         JSScope* scope = nullptr;
>+        SymbolTable* table = 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]));
>+            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--;) {
>             const ExitPropertyValue& property = materialization->properties()[i];
>@@ -139,6 +142,31 @@ extern "C" JSCell* JIT_OPERATION operati
>             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());
>+        }
>+
>         return result;
>     }
> 
>Index: Source/JavaScriptCore/tests/stress/activation-sink-default-value.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/activation-sink-default-value.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/activation-sink-default-value.js	(working copy)
>@@ -0,0 +1,31 @@
>+var n = 10000000;
>+
>+function bar(f) { f(10); }
>+
>+function foo(b) {
>+    var result = 0;
>+    var imUndefined;
>+    var baz;
>+    var set = function (x) { result = x; return (imUndefined, baz); }
>+    baz = 40;
>+    if (b) {
>+        bar(set);
>+        if (result != 10)
>+            throw "Error: bad: " + result;
>+        if (baz !== 40)
>+            throw "Error: bad: " + baz;
>+        if (imUndefined !== void 0)
>+            throw "Error: bad value: " + imUndefined;
>+        return 0;
>+    }
>+    return result;
>+}
>+
>+noInline(bar);
>+noInline(foo);
>+
>+for (var i = 0; i < n; i++) {
>+    var result = foo(!(i % 100));
>+    if (result != 0)
>+        throw "Error: bad result: " + result;
>+}
>Index: Source/JavaScriptCore/tests/stress/activation-sink-osrexit-default-value.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/activation-sink-osrexit-default-value.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/activation-sink-osrexit-default-value.js	(working copy)
>@@ -0,0 +1,37 @@
>+var n = 10000000;
>+
>+function bar(set) { 
>+    var result = set(0);
>+    if (result !== void 0)
>+        throw "Error: bad value: " + result;
>+}
>+
>+function foo(b) {
>+    var result = 0;
>+    var imUndefined;
>+    var baz;
>+    var set = function (x) { 
>+        result = x; 
>+        if (baz !== 50)
>+            throw "Error: bad value: " + baz;
>+        return imUndefined;
>+    }
>+    baz = 50;
>+    if (b) {
>+        OSRExit();
>+        if (b) {
>+            bar(set);
>+        }
>+        return 0;
>+    }
>+    return result;
>+}
>+
>+noInline(bar);
>+noInline(foo);
>+
>+for (var i = 0; i < n; i++) {
>+    var result = foo(!(i % 100));
>+    if (result != 0)
>+        throw "Error: bad result: " + result;
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 145192: 253421 | 253466 | 253566 | 253568