Bug 118338 – fourthTier: DFG should have an SSA form for use by FTL

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

[Filip Pizlo]

This will be fun.

[Filip Pizlo]

Created attachment 206181 [details]
it begins

[Filip Pizlo]

Created attachment 206182 [details]
more

[Filip Pizlo]

Created attachment 206183 [details]
more

I'm going to do phi reduction and SSA conversion in one phase.

[Filip Pizlo]

Created attachment 206196 [details]
more

[Filip Pizlo]

Created attachment 206198 [details]
more

This is starting to crystalize.

[Filip Pizlo]

Created attachment 206199 [details]
even more

[Filip Pizlo]

Created attachment 206209 [details]
reworked it again

I think I might be getting this to work.

[Filip Pizlo]

Created attachment 206214 [details]
slowly adding things

[Filip Pizlo]

Created attachment 206254 [details]
liveness analysis is done

[Filip Pizlo]

Created attachment 206282 [details]
started on CFA

[Filip Pizlo]

Created attachment 206288 [details]
started thinking about FTL backend for SSA

[Filip Pizlo]

Created attachment 206290 [details]
more

Starting to make sense of how to deal with flushed locals and their silly value formats.

[Filip Pizlo]

Created attachment 206338 [details]
getting there

Still working on constructing OSR exit data over SSA.  Doing it as a separate phase that the FTL can run before lowering.

[Filip Pizlo]

Created attachment 206407 [details]
it's getting real

OMG.  So much code.

[Filip Pizlo]

Created attachment 206408 [details]
forgot to delete a file

[Filip Pizlo]

Created attachment 206486 [details]
I might be done

[Filip Pizlo]

Created attachment 206489 [details]
more

[Filip Pizlo]

Created attachment 206503 [details]
still not compiling

[Filip Pizlo]

Created attachment 206504 [details]
OMG it compiles!

[Filip Pizlo]

Created attachment 206505 [details]
removed a file I accidentally added

[Filip Pizlo]

Created attachment 206561 [details]
added dumping of SSA metadata

[Filip Pizlo]

Created attachment 206575 [details]
it just ran something

Obviously I still have work to do but it's getting exciting.

[Filip Pizlo]

Created attachment 206586 [details]
runs some more code

[Filip Pizlo]

Created attachment 206589 [details]
it works.

[Filip Pizlo]

Comment on attachment 206589 [details]
it works.

Never mind.  This still has bugs.

[Filip Pizlo]

Created attachment 206592 [details]
it really works.

[Mark Hahnenberg]

Comment on attachment 206592 [details]
it really works.

View in context: https://bugs.webkit.org/attachment.cgi?id=206592&action=review

r=me with small changes.

> Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp:144
> +                // Three possibilities:
> +                // 1) Predecessor format is Dead, in which case it acquires our format.
> +                // 2) Predecessor format is identical to our format, in which case we
> +                //    do nothing.
> +                // 3) Predecessor format is different from our format and it's not Dead,
> +                //    in which case we have an erroneous set of Flushes and SetLocals.

What if the predecessor was already processed by the fixpoint and says "not Dead" and the current block says "Dead"?

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:260
> +        //   backwards analysis that LivenessAnalysisPhase does. As part of the

FlushLivenessAnalysisPhase?

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:338
> +                case SetArgument: {
> +                    node->setOpAndDefaultFlags(GetArgument);
> +                    node->mergeFlags(resultFor(node->variableAccessData()->flushFormat()));
> +                    break;
> +                }

Should consider keeping SetArgument for captured arguments.

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:346
> +        // Free all phis and reset variables vectors.

CPS phis?

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:373
> +                continue;

break;

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:386
> +                return edge.node();

node = edge.node();
break;

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:392
> +                return node->child1().node();

node = node->child1().node();
break;

> Source/JavaScriptCore/dfg/DFGSSAConversionPhase.h:46
> +//   After this, any remaining SetLocal means Flush. Flush nodes are
> +//   eliminated. PhantomLocals become Phantoms. Nodes may may have children

Flush nodes aren't eliminated.

"may may".

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:157
> +            dataLog("Compiling block ", *block, "\n");

Null guard for "block"? Or hoist check above.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:475
> +        MethodOfGettingAValueProfile profile =
> +            MethodOfGettingAValueProfile::fromLazyOperand(
> +                m_graph.m_profiledBlock,
> +                LazyOperandValueProfileKey(0, operand));

CodeBlock::profileForArgument?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1873
> +        ExitKind kind, FormattedValue lowValue, MethodOfGettingAValueProfile profile,
> +        LValue failCondition)
> +    {
> +        appendOSRExit(
> +            kind, lowValue, profile, failCondition, BackwardSpeculation, FormattedValue());
> +    }

Passing GetArgument as high value also works.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2467
> +        ExitKind kind, FormattedValue lowValue, MethodOfGettingAValueProfile profile,

Ditto.

[Filip Pizlo]

(In reply to comment #27)
> (From update of attachment 206592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206592&action=review
> 
> r=me with small changes.
> 
> > Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp:144
> > +                // Three possibilities:
> > +                // 1) Predecessor format is Dead, in which case it acquires our format.
> > +                // 2) Predecessor format is identical to our format, in which case we
> > +                //    do nothing.
> > +                // 3) Predecessor format is different from our format and it's not Dead,
> > +                //    in which case we have an erroneous set of Flushes and SetLocals.
> 
> What if the predecessor was already processed by the fixpoint and says "not Dead" and the current block says "Dead"?

We ain't sure about this yet and have no example of where this is needed, so I put in a FIXME for now.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:260
> > +        //   backwards analysis that LivenessAnalysisPhase does. As part of the
> 
> FlushLivenessAnalysisPhase?

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:338
> > +                case SetArgument: {
> > +                    node->setOpAndDefaultFlags(GetArgument);
> > +                    node->mergeFlags(resultFor(node->variableAccessData()->flushFormat()));
> > +                    break;
> > +                }
> 
> Should consider keeping SetArgument for captured arguments.

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:346
> > +        // Free all phis and reset variables vectors.
> 
> CPS phis?

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:373
> > +                continue;
> 
> break;

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:386
> > +                return edge.node();
> 
> node = edge.node();
> break;

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:392
> > +                return node->child1().node();
> 
> node = node->child1().node();
> break;

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGSSAConversionPhase.h:46
> > +//   After this, any remaining SetLocal means Flush. Flush nodes are
> > +//   eliminated. PhantomLocals become Phantoms. Nodes may may have children
> 
> Flush nodes aren't eliminated.

Fixed.

> 
> "may may".

Fixed.


> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:157
> > +            dataLog("Compiling block ", *block, "\n");
> 
> Null guard for "block"? Or hoist check above.

Fixed by hoisting the check.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:475
> > +        MethodOfGettingAValueProfile profile =
> > +            MethodOfGettingAValueProfile::fromLazyOperand(
> > +                m_graph.m_profiledBlock,
> > +                LazyOperandValueProfileKey(0, operand));
> 
> CodeBlock::profileForArgument?

Fixed by passing m_node as the highValue.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1873
> > +        ExitKind kind, FormattedValue lowValue, MethodOfGettingAValueProfile profile,
> > +        LValue failCondition)
> > +    {
> > +        appendOSRExit(
> > +            kind, lowValue, profile, failCondition, BackwardSpeculation, FormattedValue());
> > +    }
> 
> Passing GetArgument as high value also works.

Yup, fixed.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2467
> > +        ExitKind kind, FormattedValue lowValue, MethodOfGettingAValueProfile profile,
> 
> Ditto.

Fixed.

[Filip Pizlo]

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