WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
[patch]
more
fixflush_patch_3.diff (text/plain), 16.11 KB, created by
Filip Pizlo
on 2012-05-29 14:06:28 PDT
(
hide
)
Description:
more
Filename:
MIME Type:
Creator:
Filip Pizlo
Created:
2012-05-29 14:06:28 PDT
Size:
16.11 KB
patch
obsolete
>Index: Source/JavaScriptCore/ChangeLog >=================================================================== >--- Source/JavaScriptCore/ChangeLog (revision 118831) >+++ Source/JavaScriptCore/ChangeLog (working copy) >@@ -1,3 +1,30 @@ >+2012-05-29 Filip Pizlo <fpizlo@apple.com> >+ >+ DFG should keep captured variables alive until the (inline) return. >+ https://bugs.webkit.org/show_bug.cgi?id=87205 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Work in progress! It's starting to do things. >+ >+ * dfg/DFGByteCodeParser.cpp: >+ (JSC::DFG::ByteCodeParser::setDirect): >+ (JSC::DFG::ByteCodeParser::set): >+ (JSC::DFG::ByteCodeParser::setLocal): >+ (JSC::DFG::ByteCodeParser::getArgument): >+ (JSC::DFG::ByteCodeParser::setArgument): >+ (JSC::DFG::ByteCodeParser::flush): >+ (ByteCodeParser): >+ (JSC::DFG::ByteCodeParser::flushDirect): >+ (JSC::DFG::ByteCodeParser::flushArgumentsAndCapturedVariables): >+ (JSC::DFG::ByteCodeParser::handleInlining): >+ (JSC::DFG::ByteCodeParser::parseBlock): >+ * dfg/DFGCSEPhase.cpp: >+ (JSC::DFG::CSEPhase::setLocalStoreElimination): >+ (JSC::DFG::CSEPhase::performNodeCSE): >+ * dfg/DFGSpeculativeJIT.cpp: >+ (JSC::DFG::SpeculativeJIT::compile): >+ > 2012-05-29 Mark Hahnenberg <mhahnenberg@apple.com> > > CopiedSpace::doneCopying could start another collection >Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp >=================================================================== >--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (revision 118806) >+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (working copy) >@@ -142,20 +142,21 @@ private: > { > return getDirect(m_inlineStackTop->remapOperand(operand)); > } >- void setDirect(int operand, NodeIndex value) >+ enum SetMode { NormalSet, SetOnEntry }; >+ void setDirect(int operand, NodeIndex value, SetMode setMode = NormalSet) > { > // Is this an argument? > if (operandIsArgument(operand)) { >- setArgument(operand, value); >+ setArgument(operand, value, setMode); > return; > } > > // Must be a local. >- setLocal((unsigned)operand, value); >+ setLocal((unsigned)operand, value, setMode); > } >- void set(int operand, NodeIndex value) >+ void set(int operand, NodeIndex value, SetMode setMode = NormalSet) > { >- setDirect(m_inlineStackTop->remapOperand(operand), value); >+ setDirect(m_inlineStackTop->remapOperand(operand), value, setMode); > } > > NodeIndex injectLazyOperandPrediction(NodeIndex nodeIndex) >@@ -236,17 +237,13 @@ private: > > return nodeIndex; > } >- void setLocal(unsigned operand, NodeIndex value) >+ void setLocal(unsigned operand, NodeIndex value, SetMode setMode = NormalSet) > { > bool isCaptured = m_codeBlock->localIsCaptured(m_inlineStackTop->m_inlineCallFrame, operand); > >- VariableAccessData* variableAccessData = newVariableAccessData(operand, isCaptured); >- NodeIndex nodeIndex = addToGraph(SetLocal, OpInfo(variableAccessData), value); >- m_currentBlock->variablesAtTail.local(operand) = nodeIndex; >- >- bool shouldFlush = isCaptured; >+ if (setMode == NormalSet) { >+ bool shouldFlush = isCaptured; > >- if (!shouldFlush) { > // If this is in argument position, then it should be flushed. > for (InlineStackEntry* stack = m_inlineStackTop; ; stack = stack->m_caller) { > InlineCallFrame* inlineCallFrame = stack->m_inlineCallFrame; >@@ -259,14 +256,18 @@ private: > if (operand < inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) > continue; > int argument = operandToArgument(operand - inlineCallFrame->stackOffset); >- stack->m_argumentPositions[argument]->addVariable(variableAccessData); >- shouldFlush = true; >+ stack->m_argumentPositions[argument]->addVariable(flushDirect(operand)); >+ shouldFlush = false; > break; > } >- } > >- if (shouldFlush) >- addToGraph(Flush, OpInfo(variableAccessData), nodeIndex); >+ if (shouldFlush) >+ flushDirect(operand); >+ } >+ >+ VariableAccessData* variableAccessData = newVariableAccessData(operand, isCaptured); >+ NodeIndex nodeIndex = addToGraph(SetLocal, OpInfo(variableAccessData), value); >+ m_currentBlock->variablesAtTail.local(operand) = nodeIndex; > } > > // Used in implementing get/set, above, where the operand is an argument. >@@ -292,7 +293,7 @@ private: > VariableAccessData* variableAccessData = flushChild.variableAccessData(); > variableAccessData->mergeIsCaptured(isCaptured); > nodeIndex = injectLazyOperandPrediction(addToGraph(GetLocal, OpInfo(variableAccessData), nodeIndex)); >- m_currentBlock->variablesAtTail.local(operand) = nodeIndex; >+ m_currentBlock->variablesAtTail.argument(argument) = nodeIndex; > return nodeIndex; > } > nodePtr = &flushChild; >@@ -336,31 +337,36 @@ private: > > return nodeIndex; > } >- void setArgument(int operand, NodeIndex value) >+ void setArgument(int operand, NodeIndex value, SetMode setMode = NormalSet) > { > unsigned argument = operandToArgument(operand); > bool isCaptured = m_codeBlock->argumentIsCaptured(argument); > > ASSERT(argument < m_numArguments); > >+ // Always flush arguments, except for 'this'. >+ if (argument && setMode == NormalSet) { >+ InlineStackEntry* stack = m_inlineStackTop; >+ while (stack->m_inlineCallFrame) // find the machine stack entry. >+ stack = stack->m_caller; >+ stack->m_argumentPositions[argument]->addVariable(flushDirect(operand)); >+ } >+ > VariableAccessData* variableAccessData = newVariableAccessData(operand, isCaptured); >- InlineStackEntry* stack = m_inlineStackTop; >- while (stack->m_inlineCallFrame) // find the machine stack entry. >- stack = stack->m_caller; >- stack->m_argumentPositions[argument]->addVariable(variableAccessData); > NodeIndex nodeIndex = addToGraph(SetLocal, OpInfo(variableAccessData), value); > m_currentBlock->variablesAtTail.argument(argument) = nodeIndex; >- // Always flush arguments, except for 'this'. >- if (argument) >- addToGraph(Flush, OpInfo(variableAccessData), nodeIndex); > } > >- VariableAccessData* flushArgument(int operand) >+ VariableAccessData* flush(int operand) >+ { >+ return flushDirect(m_inlineStackTop->remapOperand(operand)); >+ } >+ >+ VariableAccessData* flushDirect(int operand) > { > // FIXME: This should check if the same operand had already been flushed to > // some other local variable. > >- operand = m_inlineStackTop->remapOperand(operand); > bool isCaptured = m_codeBlock->isCaptured(m_inlineStackTop->m_inlineCallFrame, operand); > > ASSERT(operand < FirstConstantRegisterIndex); >@@ -415,6 +421,19 @@ private: > } > return variableAccessData; > } >+ >+ void flushArgumentsAndCapturedVariables() >+ { >+ int numArguments; >+ if (m_inlineStackTop->m_inlineCallFrame) >+ numArguments = m_inlineStackTop->m_inlineCallFrame->arguments.size(); >+ else >+ numArguments = m_inlineStackTop->m_codeBlock->numParameters(); >+ for (unsigned argument = numArguments; argument-- > 1;) >+ flush(argumentToOperand(argument)); >+ for (unsigned local = m_inlineStackTop->m_codeBlock->m_numCapturedVars; local--;) >+ flush(local); >+ } > > // Get an operand, and perform a ToInt32/ToNumber conversion on it. > NodeIndex getToInt32(int operand) >@@ -1219,17 +1238,6 @@ bool ByteCodeParser::handleInlining(bool > > // FIXME: Don't flush constants! > >- Vector<VariableAccessData*, 8> arguments; >- for (int i = 1; i < argumentCountIncludingThis; ++i) { >- VariableAccessData* variableAccessData = >- flushArgument(registerOffset + argumentToOperand(i)); >- arguments.append(variableAccessData); >- >- // Are we going to be capturing arguments? If so make sure we record this fact. >- if (codeBlock->argumentIsCaptured(i)) >- variableAccessData->mergeIsCaptured(true); >- } >- > int inlineCallFrameStart = m_inlineStackTop->remapOperand(registerOffset) - RegisterFile::CallFrameHeaderSize; > > // Make sure that the area used by the call frame is reserved. >@@ -1251,13 +1259,6 @@ bool ByteCodeParser::handleInlining(bool > usesResult ? resultOperand : InvalidVirtualRegister), > (VirtualRegister)inlineCallFrameStart, argumentCountIncludingThis, kind); > >- // Link up the argument variable access datas to their argument positions. >- for (int i = 1; i < argumentCountIncludingThis; ++i) { >- if (static_cast<size_t>(i) >= inlineStackEntry.m_argumentPositions.size()) >- break; >- inlineStackEntry.m_argumentPositions[i]->addVariable(arguments[i - 1]); >- } >- > // This is where the actual inlining really happens. > unsigned oldIndex = m_currentIndex; > unsigned oldProfilingIndex = m_currentProfilingIndex; >@@ -1578,7 +1579,7 @@ bool ByteCodeParser::parseBlock(unsigned > case op_enter: > // Initialize all locals to undefined. > for (int i = 0; i < m_inlineStackTop->m_codeBlock->m_numVars; ++i) >- set(i, constantUndefined()); >+ set(i, constantUndefined(), SetOnEntry); > NEXT_OPCODE(op_enter); > > case op_convert_this: { >@@ -2352,6 +2353,7 @@ bool ByteCodeParser::parseBlock(unsigned > } > > case op_ret: >+ flushArgumentsAndCapturedVariables(); > if (m_inlineStackTop->m_inlineCallFrame) { > if (m_inlineStackTop->m_returnValue != InvalidVirtualRegister) > setDirect(m_inlineStackTop->m_returnValue, get(currentInstruction[1].u.operand)); >@@ -2379,6 +2381,7 @@ bool ByteCodeParser::parseBlock(unsigned > LAST_OPCODE(op_ret); > > case op_end: >+ flushArgumentsAndCapturedVariables(); > ASSERT(!m_inlineStackTop->m_inlineCallFrame); > addToGraph(Return, get(currentInstruction[1].u.operand)); > LAST_OPCODE(op_end); >Index: Source/JavaScriptCore/dfg/DFGCSEPhase.cpp >=================================================================== >--- Source/JavaScriptCore/dfg/DFGCSEPhase.cpp (revision 118806) >+++ Source/JavaScriptCore/dfg/DFGCSEPhase.cpp (working copy) >@@ -1,5 +1,5 @@ > /* >- * Copyright (C) 2011 Apple Inc. All rights reserved. >+ * Copyright (C) 2011, 2012 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions >@@ -620,7 +620,7 @@ private: > } > > // This returns the Flush node that is keeping a SetLocal alive. >- NodeIndex setLocalStoreElimination(VirtualRegister local) >+ NodeIndex setLocalStoreElimination(VirtualRegister local, NodeIndex expectedNodeIndex) > { > for (unsigned i = m_indexInBlock; i--;) { > NodeIndex index = m_currentBlock->at(i); >@@ -629,7 +629,7 @@ private: > continue; > switch (node.op()) { > case GetLocal: >- case SetLocal: >+ case Flush: > if (node.local() == local) > return NoNode; > break; >@@ -639,19 +639,13 @@ private: > return NoNode; > break; > >- case Flush: { >+ case SetLocal: { > if (node.local() != local) > break; >- if (!i) >- break; >- NodeIndex prevIndex = m_currentBlock->at(i - 1); >- if (prevIndex != node.child1().index()) >- break; >- ASSERT(m_graph[prevIndex].local() == local); >- ASSERT(m_graph[prevIndex].variableAccessData() == node.variableAccessData()); >- ASSERT(m_graph[prevIndex].shouldGenerate()); >- if (m_graph[prevIndex].refCount() > 1) >- break; >+ if (index != expectedNodeIndex) >+ return NoNode; >+ if (m_graph[index].refCount() > 1) >+ return NoNode; > return index; > } > >@@ -660,6 +654,11 @@ private: > return NoNode; > break; > >+ case TearOffActivation: >+ // If an activation is being torn off then it means that captured variables >+ // are live. >+ return NoNode; >+ > default: > if (m_graph.clobbersWorld(index)) > return NoNode; >@@ -855,27 +854,26 @@ private: > break; > } > >- case SetLocal: { >+ case Flush: { > if (m_fixpointState == FixpointNotConverged) > break; > VariableAccessData* variableAccessData = node.variableAccessData(); > if (!variableAccessData->isCaptured()) > break; > VirtualRegister local = variableAccessData->local(); >- NodeIndex replacementIndex = setLocalStoreElimination(local); >+ NodeIndex replacementIndex = setLocalStoreElimination(local, node.child1().index()); > if (replacementIndex == NoNode) > break; > Node& replacement = m_graph[replacementIndex]; >- ASSERT(replacement.op() == Flush); >+ ASSERT(replacement.op() == SetLocal); > ASSERT(replacement.refCount() == 1); > ASSERT(replacement.shouldGenerate()); >- ASSERT(replacement.mustGenerate()); >- replacement.setOpAndDefaultFlags(Phantom); >- NodeIndex setLocalIndex = replacement.child1().index(); >- ASSERT(m_graph[setLocalIndex].op() == SetLocal); >- m_graph.clearAndDerefChild1(replacement); >- replacement.children.child1() = m_graph[setLocalIndex].child1(); >- m_graph.ref(replacement.child1()); >+ node.setOpAndDefaultFlags(Phantom); >+ NodeIndex dataNodeIndex = replacement.child1().index(); >+ ASSERT(m_graph[dataNodeIndex].hasResult()); >+ m_graph.clearAndDerefChild1(node); >+ node.children.child1() = Edge(dataNodeIndex); >+ m_graph.ref(dataNodeIndex); > break; > } > >Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp >=================================================================== >--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp (revision 118806) >+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp (working copy) >@@ -1000,14 +1000,12 @@ void SpeculativeJIT::compile(BasicBlock& > ASSERT(m_variables.size() == block.variablesAtHead.numberOfLocals()); > for (size_t i = 0; i < m_variables.size(); ++i) { > NodeIndex nodeIndex = block.variablesAtHead.local(i); >- // FIXME: Use the variable access data, not the first node in the block. >- // https://bugs.webkit.org/show_bug.cgi?id=87205 >- if (m_jit.codeBlock()->localIsCaptured(at(block[0]).codeOrigin.inlineCallFrame, i)) >- m_variables[i] = ValueSource(ValueInRegisterFile); >- else if (nodeIndex == NoNode) >+ if (nodeIndex == NoNode) > m_variables[i] = ValueSource(SourceIsDead); > else if (at(nodeIndex).variableAccessData()->isArgumentsAlias()) > m_variables[i] = ValueSource(ArgumentsSource); >+ else if (at(nodeIndex).variableAccessData()->isCaptured()) >+ m_variables[i] = ValueSource(ValueInRegisterFile); > else if (!at(nodeIndex).refCount()) > m_variables[i] = ValueSource(SourceIsDead); > else if (at(nodeIndex).variableAccessData()->shouldUseDoubleFormat())
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=144616">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 87205
:
144458
|
144464
|
144616
|
144629
|
144638