Bug 126778

Created attachment 251134 [details] work in progress

Created attachment 251209 [details] the patch

Attachment 251209 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.h:121: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGValidate.cpp:202: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 251209 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=251209&action=review Please fix style issues. r=me with suggestions addressed. > Source/JavaScriptCore/dfg/DFGBasicBlock.h:313 > private: > + > friend class InsertionSet; Unnecessary blank line added here. Please remove. > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:339 > + size_t upsilonInsertionPoint = block->findTerminal().index; > + NodeOrigin upsilonOrigin = block->terminal()->origin; block->terminal() will invoke findTerminal() again. Why not just cache and use the NodeAndIndex result? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1932 > - case MovHint: > - case ZombieHint: { > + case MovHint: { > DFG_CRASH(m_jit.graph(), node, "Unexpected node"); > break; > } > + > + case ZombieHint: { > + recordSetLocal(m_currentNode->unlinkedLocal(), VirtualRegister(), DataFormatDead); > + break; > + } This part is already in ToT, right? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-1449 > - case ZombieHint: { > - recordSetLocal(m_currentNode->unlinkedLocal(), VirtualRegister(), DataFormatDead); > - break; > - } > - Ditto. Already in ToT. > Source/JavaScriptCore/dfg/DFGTierUpCheckInjectionPhase.cpp:109 > + if (block->terminal()->op() == Return) { > insertionSet.insertNode( > - block->size() - 1, SpecNone, CheckTierUpAtReturn, block->last()->origin); > + block->findTerminal().index, SpecNone, CheckTierUpAtReturn, > + block->terminal()->origin); Again, you're calling findTerminal() 3 times here. Why not cache and reuse the NodeAndIndex?

Comment on attachment 251209 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=251209&action=review > Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:155 > + if (block->terminal()->child1()->hasConstant()) { > + FrozenValue* value = block->terminal()->child1()->constant(); Ditto, can call findTerminal() and cache here. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:465 > + size_t upsilonInsertionPoint = block->findTerminal().index; > + Node* upsilonWhere = block->terminal(); Ditto. Use findTerminal() once? > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:712 > + size_t upsilonInsertionPoint = block->findTerminal().index; > + NodeOrigin upsilonOrigin = block->terminal()->origin; Ditto. Use findTerminal() once? > Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:445 > + size_t upsilonInsertionPoint = block->findTerminal().index; > + NodeOrigin upsilonOrigin = block->terminal()->origin; Ditto. Use findTerminal() once? > Source/JavaScriptCore/dfg/DFGStaticExecutionCountEstimationPhase.cpp:66 > + switch (block->terminal()->op()) { Ditto. Use findTerminal() once?

I fixed all of the repeated terminal() calls locally.

(In reply to comment #4) > Comment on attachment 251209 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251209&action=review > > Please fix style issues. > > r=me with suggestions addressed. > > > Source/JavaScriptCore/dfg/DFGBasicBlock.h:313 > > private: > > + > > friend class InsertionSet; > > Unnecessary blank line added here. Please remove. > > > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:339 > > + size_t upsilonInsertionPoint = block->findTerminal().index; > > + NodeOrigin upsilonOrigin = block->terminal()->origin; > > block->terminal() will invoke findTerminal() again. Why not just cache and > use the NodeAndIndex result? > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1932 > > - case MovHint: > > - case ZombieHint: { > > + case MovHint: { > > DFG_CRASH(m_jit.graph(), node, "Unexpected node"); > > break; > > } > > + > > + case ZombieHint: { > > + recordSetLocal(m_currentNode->unlinkedLocal(), VirtualRegister(), DataFormatDead); > > + break; > > + } > > This part is already in ToT, right? Correct - this was the part of what I landed in an earlier patch that I needed to write this patch. > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-1449 > > - case ZombieHint: { > > - recordSetLocal(m_currentNode->unlinkedLocal(), VirtualRegister(), DataFormatDead); > > - break; > > - } > > - > > Ditto. Already in ToT. > > > Source/JavaScriptCore/dfg/DFGTierUpCheckInjectionPhase.cpp:109 > > + if (block->terminal()->op() == Return) { > > insertionSet.insertNode( > > - block->size() - 1, SpecNone, CheckTierUpAtReturn, block->last()->origin); > > + block->findTerminal().index, SpecNone, CheckTierUpAtReturn, > > + block->terminal()->origin); > > Again, you're calling findTerminal() 3 times here. Why not cache and reuse > the NodeAndIndex?

Landed in http://trac.webkit.org/changeset/183094