[webkit-reviews] review granted: [Bug 179762] [FTL] NewArrayBuffer should be sinked if it is only used for spreading : [Attachment 328801] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 17 16:17:47 PST 2017


Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 179762: [FTL] NewArrayBuffer should be sinked if it is only used for
spreading
https://bugs.webkit.org/show_bug.cgi?id=179762

Attachment 328801: Patch

https://bugs.webkit.org/attachment.cgi?id=328801&action=review




--- Comment #26 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 328801
  --> https://bugs.webkit.org/attachment.cgi?id=328801
Patch

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

r=me
Some comments and suggestions for tests for the cases where you may not have
them.

> Source/WTF/ChangeLog:8
> +	   We add RecurableLambda<>. This can take a lambda and offer a way

This should be "RecursableLambda" everywhere in this patch. Both upper and
lower case.

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:850
> +			   if (canConvertToStaticLoadStores(candidate)) {

do you have tests for both this being true and false?

> Source/JavaScriptCore/dfg/DFGValidate.cpp:763
> +		       VALIDATE((node), node->child1()->op() ==
PhantomCreateRest || node->child1()->op() == PhantomNewArrayBuffer);

You need to update the Spread rule below to say we support
Spread(PhantomNewArrayBuffer). It'd be great if you can write a test for it
too. Maybe you can by looking at the code I added to originally support 
Spread(PhantomCreateRest)

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5194
> +				   // Because resulted array from
NewArrayWithSpread is always contiguous, we should not generate value
> +				   // in Double form even if
PhantomNewArrayBuffer's indexingType is ArrayWithDouble.

Did you add tests for this?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5196
> +				   m_out.store64(m_out.constInt64(value),
m_out.baseIndex(heap, storage, index, JSValue(), i * sizeof(JSValue)));

Maybe it's worth being paranoid about (i*sizeof(JSValue)) overflowing here? We
can just crash if it does.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5316
> +	       setJSValue(frozenPointer(m_node->child1()->cellOperand()));

nice! Please add a test if possible as state in comment about Validation phase.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7296
> +		       auto emitArgumentsFromRightToLeft =
recurableLambda([&](auto self, Node* target) -> void {

Do you have tests that mix all three kinds of phantoms in a single call?
PhantomCreateRest, PhantomNewArrayBuffer, and just normal values. If not please
add.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7323
> +				  
jit.subPtr(CCallHelpers::TrustedImmPtr(static_cast<size_t>(1)), scratchGPR2);

I would vote for doing this outside the loop and adjusting the store below to
do the right thing.

> Source/WTF/wtf/RecurableLambda.h:33
> +class RecurableLambda {

make this: RecursableLambda

> Source/WTF/wtf/RecurableLambda.h:51
> +decltype(auto) recurableLambda(Functor&& f)

make this: recursableLambda


More information about the webkit-reviews mailing list