[webkit-reviews] review granted: [Bug 118338] fourthTier: DFG should have an SSA form for use by FTL : [Attachment 206592] it really works.

[bugzilla-daemon at webkit.org]

Mark Hahnenberg <mhahnenberg at apple.com> has granted Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 118338: fourthTier: DFG should have an SSA form for use by FTL
https://bugs.webkit.org/show_bug.cgi?id=118338

Attachment 206592: it really works.
https://bugs.webkit.org/attachment.cgi?id=206592&action=review

------- Additional Comments from Mark Hahnenberg <mhahnenberg at apple.com>
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.