[webkit-reviews] review granted: [Bug 126040] Add support for StoreBarrier and friends to the FTL : [Attachment 219808] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 126040: Add support for StoreBarrier and friends to the FTL
https://bugs.webkit.org/show_bug.cgi?id=126040

Attachment 219808: Patch
https://bugs.webkit.org/attachment.cgi?id=219808&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219808&action=review


> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:605
> +	   DFG_NODE_DO_TO_CHILDREN(m_graph, m_node, speculate);

Might as well just say: speculate(m_node->child1()).  Maybe you also have to
pass m_node, I don't remember the signature.

But maybe it would be even better to say:

compileStoreBarrier(lowCell(m_node->child1()));

And then have the #if ENABLE(GGC) inside compileStoreBarrier().  You can rely
on lowCell() doing the right thing if the result is unused.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:612
> +	   compileBaseValueStoreBarrier(m_node->child1(), m_node->child2());

Does this helper buy anything?	I think you call it only here.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:614
> +	   DFG_NODE_DO_TO_CHILDREN(m_graph, m_node, speculate);

Might as well just say:

speculate(m_node->child1());
specualte(m_node->child2());

But see above.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:630
> +	   DFG_NODE_DO_TO_CHILDREN(m_graph, m_node, speculate);

See above.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4066
> +    void compileStoreBarrier(LValue base, LValue value, Edge& valueEdge)

Rename to emitStoreBarrier.  We use the term "compileBlah" to mean that Blah is
a node type that we're lowering.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4083
> +    void compileStoreBarrier(LValue base)

Rename to "emitStoreBarrier".

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4114
> +    void compileBaseValueStoreBarrier(Edge& baseEdge, Edge& valueEdge)

You probably don't need this helper.