Summary: | DFG should allow Phantoms after terminals | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | barraclough, commit-queue, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 143735, 145134 | ||||||||
Attachments: |
|
Description
Filip Pizlo
2014-01-10 13:43:52 PST
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 |