Attachment 340009 Details for Bug 185452 – possible patch

[patch] possible patch

blah.patch (text/plain), 12.54 KB, created by Filip Pizlo on 2018-05-09 12:50:43 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2018-05-09 12:50:43 PDT

Size: 12.54 KB

patch

obsolete

>Index: Source/JavaScriptCore/bytecode/Operands.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/Operands.h	(revision 231522)
>+++ Source/JavaScriptCore/bytecode/Operands.h	(working copy)
>@@ -71,19 +71,28 @@ public:
>     size_t numberOfArguments() const { return m_numArguments; }
>     size_t numberOfLocals() const { return m_values.size() - m_numArguments; }
>     
>-    T& argument(size_t idx)
>+    size_t argumentIndex(size_t idx) const
>     {
>         ASSERT(idx < m_numArguments);
>-        return m_values[idx];
>+        return idx;
>+    }
>+    
>+    size_t localIndex(size_t idx) const
>+    {
>+        return m_numArguments + idx;
>+    }
>+    
>+    T& argument(size_t idx)
>+    {
>+        return m_values[argumentIndex(idx)];
>     }
>     const T& argument(size_t idx) const
>     {
>-        ASSERT(idx < m_numArguments);
>-        return m_values[idx];
>+        return m_values[argumentIndex(idx)];
>     }
>     
>-    T& local(size_t idx) { return m_values[m_numArguments + idx]; }
>-    const T& local(size_t idx) const { return m_values[m_numArguments + idx]; }
>+    T& local(size_t idx) { return m_values[localIndex(idx)]; }
>+    const T& local(size_t idx) const { return m_values[localIndex(idx)]; }
>     
>     template<OperandKind operandKind>
>     size_t sizeFor() const
>@@ -156,6 +165,18 @@ public:
>         setLocal(idx, value);
>     }
>     
>+    size_t operandIndex(int operand) const
>+    {
>+        if (operandIsArgument(operand))
>+            return argumentIndex(VirtualRegister(operand).toArgument());
>+        return localIndex(VirtualRegister(operand).toLocal());
>+    }
>+    
>+    size_t operandIndex(VirtualRegister virtualRegister) const
>+    {
>+        return operandIndex(virtualRegister.offset());
>+    }
>+    
>     T& operand(int operand)
>     {
>         if (operandIsArgument(operand))
>Index: Source/JavaScriptCore/dfg/DFGAbstractValue.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGAbstractValue.h	(revision 231522)
>+++ Source/JavaScriptCore/dfg/DFGAbstractValue.h	(working copy)
>@@ -104,6 +104,22 @@ struct AbstractValue {
>         checkConsistency();
>     }
>     
>+    ALWAYS_INLINE void fastForwardFromTo(AbstractValueClobberEpoch oldEpoch, AbstractValueClobberEpoch newEpoch)
>+    {
>+        if (newEpoch == m_effectEpoch)
>+            return;
>+        
>+        if (!(m_type & SpecCell))
>+            return;
>+
>+        if (newEpoch.clobberEpoch() != oldEpoch.clobberEpoch())
>+            clobberStructures();
>+        if (newEpoch.structureClobberState() == StructuresAreWatched)
>+            m_structure.observeInvalidationPoint();
>+
>+        checkConsistency();
>+    }
>+    
>     ALWAYS_INLINE void fastForwardTo(AbstractValueClobberEpoch newEpoch)
>     {
>         if (newEpoch == m_effectEpoch)
>Index: Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp	(revision 231522)
>+++ Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp	(working copy)
>@@ -54,31 +54,22 @@ InPlaceAbstractState::~InPlaceAbstractSt
> 
> void InPlaceAbstractState::beginBasicBlock(BasicBlock* basicBlock)
> {
>-    // This function is ~1.6-2% of execution time.
>-    
>     ASSERT(!m_block);
>     
>     ASSERT(basicBlock->variablesAtHead.numberOfLocals() == basicBlock->valuesAtHead.numberOfLocals());
>     ASSERT(basicBlock->variablesAtTail.numberOfLocals() == basicBlock->valuesAtTail.numberOfLocals());
>     ASSERT(basicBlock->variablesAtHead.numberOfLocals() == basicBlock->variablesAtTail.numberOfLocals());
> 
>-    m_abstractValues.resize(); // This part is ~0.1-0.4% of execution time.
>+    m_abstractValues.resize();
> 
>     AbstractValueClobberEpoch epoch = AbstractValueClobberEpoch::first(basicBlock->cfaStructureClobberStateAtHead);
>     m_effectEpoch = epoch;
> 
>-    // This loop is 0.9-1.2% of execution time.
>-    // FIXME: Lazily populate m_variables when GetLocal/SetLocal happens. Apply the same idea to
>-    // merging. Alternatively, we could just use liveness here.
>-    // https://bugs.webkit.org/show_bug.cgi?id=185452
>-    for (size_t i = m_variables.size(); i--;) {
>-        AbstractValue& value = m_variables[i];
>-        value = basicBlock->valuesAtHead[i];
>-        value.m_effectEpoch = epoch;
>-    }
>+    m_activeVariables.clearRange(0, std::min(m_variables.size(), m_activeVariables.size()));
>+    if (m_variables.size() < m_activeVariables.size())
>+        m_activeVariables.resize(m_variables.size());
>     
>     if (m_graph.m_form == SSA) {
>-        // This loop is 0.05-0.17% of execution time.
>         for (NodeAbstractValuePair& entry : basicBlock->ssa->valuesAtHead) {
>             if (entry.node.isStillValid()) {
>                 AbstractValue& value = m_abstractValues.at(entry.node);
>@@ -104,6 +95,13 @@ static void setLiveValues(Vector<NodeAbs
>         values.uncheckedAppend(NodeAbstractValuePair { node, AbstractValue() });
> }
> 
>+Operands<AbstractValue>& InPlaceAbstractState::variablesForDebugging()
>+{
>+    for (size_t i = m_variables.size(); i--;)
>+        activateVariableIfNecessary(i);
>+    return m_variables;
>+}
>+
> void InPlaceAbstractState::initialize()
> {
>     for (BasicBlock* entrypoint : m_graph.m_roots) {
>@@ -207,24 +205,69 @@ bool InPlaceAbstractState::endBasicBlock
>     }
> 
>     block->cfaStructureClobberStateAtTail = m_structureClobberState;
>+    
>+    AbstractValueClobberEpoch epochAtHead =
>+        AbstractValueClobberEpoch::first(block->cfaStructureClobberStateAtHead);
>+    AbstractValueClobberEpoch currentEpoch = m_effectEpoch;
> 
>     switch (m_graph.m_form) {
>     case ThreadedCPS: {
>-        for (size_t argument = 0; argument < block->variablesAtTail.numberOfArguments(); ++argument) {
>-            AbstractValue& destination = block->valuesAtTail.argument(argument);
>-            mergeStateAtTail(destination, this->argument(argument), block->variablesAtTail.argument(argument));
>-        }
>-
>-        for (size_t local = 0; local < block->variablesAtTail.numberOfLocals(); ++local) {
>-            AbstractValue& destination = block->valuesAtTail.local(local);
>-            mergeStateAtTail(destination, this->local(local), block->variablesAtTail.local(local));
>+        ASSERT(block->variablesAtTail.size() == block->valuesAtTail.size());
>+        ASSERT(block->variablesAtTail.size() == m_variables.size());
>+        for (size_t index = block->variables.size(); index--;) {
>+            Node* node = block->variablesAtTail[index];
>+            if (!node)
>+                continue;
>+            AbstractValue& destination = block->valuesAtTail[index];
>+            
>+            if (!m_activeVariables.get(index)) {
>+                destination = block->valuesAtHead[index];
>+                destination.fastForwardFromTo(epochAtHead, currentEpoch);
>+                continue;
>+            }
>+            
>+            switch (node->op()) {
>+            case Phi:
>+            case SetArgument:
>+            case PhantomLocal:
>+            case Flush:
>+                // The block transfers the value from head to tail.
>+                destination = variableAt(index);
>+                break;
>+                
>+            case GetLocal:
>+                // The block refines the value with additional speculations.
>+                destination = forNode(node);
>+                break;
>+                
>+            case SetLocal:
>+                // The block sets the variable, and potentially refines it, both
>+                // before and after setting it.
>+                destination = forNode(node->child1());
>+                if (node->variableAccessData()->flushFormat() == FlushedDouble)
>+                    RELEASE_ASSERT(!(source->m_type & ~SpecFullDouble));
>+                break;
>+                
>+            default:
>+                RELEASE_ASSERT_NOT_REACHED();
>+                break;
>+            }
>         }
>         break;
>     }
> 
>     case SSA: {
>-        for (size_t i = 0; i < block->valuesAtTail.size(); ++i)
>+        for (size_t i = 0; i < block->valuesAtTail.size(); ++i) {
>+            AbstractValue& destination = block->valuesAtTail[i];
>+
>+            if (!m_activeVariables.get(i)) {
>+                destination = block->valuesAtHead[i];
>+                destination.fastForwardFromTo(epochAtHead, currentEpoch);
>+                continue;
>+            }
>+            
>             block->valuesAtTail[i] = variableAt(i);
>+        }
> 
>         for (NodeAbstractValuePair& valueAtTail : block->ssa->valuesAtTail)
>             valueAtTail.value = forNode(valueAtTail.node);
>@@ -248,40 +291,12 @@ void InPlaceAbstractState::reset()
>     m_structureClobberState = StructuresAreWatched;
> }
> 
>-void InPlaceAbstractState::mergeStateAtTail(AbstractValue& destination, AbstractValue& inVariable, Node* node)
>+void InPlaceAbstractState::activateVariable(size_t variableIndex)
> {
>-    if (!node)
>-        return;
>-
>-    const AbstractValue* source = nullptr;
>-    
>-    switch (node->op()) {
>-    case Phi:
>-    case SetArgument:
>-    case PhantomLocal:
>-    case Flush:
>-        // The block transfers the value from head to tail.
>-        source = &inVariable;
>-        break;
>-            
>-    case GetLocal:
>-        // The block refines the value with additional speculations.
>-        source = &forNode(node);
>-        break;
>-            
>-    case SetLocal:
>-        // The block sets the variable, and potentially refines it, both
>-        // before and after setting it.
>-        source = &forNode(node->child1());
>-        if (node->variableAccessData()->flushFormat() == FlushedDouble)
>-            RELEASE_ASSERT(!(source->m_type & ~SpecFullDouble));
>-        break;
>-        
>-    default:
>-        RELEASE_ASSERT_NOT_REACHED();
>-        break;
>-    }
>-    destination = *source;
>+    AbstractValue& value = m_variables[variableIndex];
>+    value = m_block->valuesAtHead[variableIndex];
>+    value.m_effectEpoch = AbstractValueClobberEpoch::first(m_block->cfaStructureClobberStateAtHead);
>+    m_activeVariables.set(variableIndex);
> }
> 
> bool InPlaceAbstractState::merge(BasicBlock* from, BasicBlock* to)
>Index: Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h	(revision 231522)
>+++ Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h	(working copy)
>@@ -135,15 +135,33 @@ public:
>         makeHeapTopForNode(edge.node());
>     }
>     
>-    Operands<AbstractValue>& variablesForDebugging() { return m_variables; }
>+    Operands<AbstractValue>& variablesForDebugging();
> 
>     unsigned numberOfArguments() const { return m_variables.numberOfArguments(); }
>     unsigned numberOfLocals() const { return m_variables.numberOfLocals(); }
>-    AbstractValue& operand(int operand) { return fastForward(m_variables.operand(operand)); }
>-    AbstractValue& operand(VirtualRegister operand) { return fastForward(m_variables.operand(operand)); }
>-    AbstractValue& local(size_t index) { return fastForward(m_variables.local(index)); }
>-    AbstractValue& argument(size_t index) { return fastForward(m_variables.argument(index)); }
>-    AbstractValue& variableAt(size_t index) { return fastForward(m_variables[index]); }
>+    
>+    AbstractValue& variableAt(size_t index)
>+    {
>+        activateVariableIfNecessary(index);
>+        return fastForward(m_variables[index]);
>+    }
>+
>+    AbstractValue& operand(int operand)
>+    {
>+        return variableAt(m_variables.operandIndex(operand));
>+    }
>+    
>+    AbstractValue& operand(VirtualRegister operand) { return operand(operand.offset()); }
>+    
>+    AbstractValue& local(size_t index)
>+    {
>+        return variableAt(m_variables.localIndex(index));
>+    }
>+    
>+    AbstractValue& argument(size_t index)
>+    {
>+        return variableAt(m_variables.argumentIndex(index));
>+    }
>     
>     // Call this before beginning CFA to initialize the abstract values of
>     // arguments, and to indicate which blocks should be listed for CFA
>@@ -232,14 +250,21 @@ private:
>         return value;
>     }
>     
>-    void mergeStateAtTail(AbstractValue& destination, AbstractValue& inVariable, Node*);
>-
>+    ALWAYS_INLINE void activateVariableIfNecessary(size_t variableIndex)
>+    {
>+        if (!m_activeVariables.get(variableIndex))
>+            activateVariable(variableIndex);
>+    }
>+    
>+    void activateVariable(size_t variableIndex);
>+    
>     static bool mergeVariableBetweenBlocks(AbstractValue& destination, AbstractValue& source, Node* destinationNode, Node* sourceNode);
>     
>     Graph& m_graph;
> 
>     FlowMap<AbstractValue>& m_abstractValues;
>     Operands<AbstractValue> m_variables;
>+    FastBitVector m_activeVariables;
>     BasicBlock* m_block;
>     
>     bool m_foundConstants;

Actions: View | Formatted Diff | Diff

Attachments on bug 185452: 340009 | 340048 | 340050 | 340061 | 340103 | 340121