Bug 126778 – DFG should allow Phantoms after terminals

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

This would allow us to get rid of some hacks and would also enable new kinds of CFG simplification.

[Filip Pizlo]

Created attachment 251134 [details]
work in progress

[Filip Pizlo]

Created attachment 251209 [details]
the patch

[WebKit Commit Bot]

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.

[Mark Lam]

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?

[Mark Lam]

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?

[Filip Pizlo]

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

[Filip Pizlo]

(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?

[Filip Pizlo]

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