[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