[webkit-reviews] review denied: [Bug 179762] [FTL] NewArrayBuffer should be sinked if it is only used for spreading : [Attachment 327300] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Nov 26 16:51:09 PST 2017
Saam Barati <sbarati at apple.com> has denied 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 327300: Patch
https://bugs.webkit.org/attachment.cgi?id=327300&action=review
--- Comment #19 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 327300
--> https://bugs.webkit.org/attachment.cgi?id=327300
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=327300&action=review
r- because I think I found a bug w.r.t double representation. I'd like to see
some tests for that if it's indeed a bug, and I'll re-review. Also some various
other comments.
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5110
> + if (use->child1()->op() == PhantomNewArrayBuffer)
> + lengthCheck = m_out.speculateAdd(length,
m_out.constInt32(use->child1()->numConstants()));
This should be done as the "startLength" calculation above. You should also
probably add an overflow check to that math to be extra safe.
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5144
> + if
(hasDouble(use->child1()->indexingType()))
> + value =
bitwise_cast<int64_t>(data[i].asNumber());
This is wrong. Please add a test for it. We're allocating a contiguous array
here. We can not store doubles into it.
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5150
> + index = m_out.add(index,
m_out.constIntPtr(1));
Let's pull this out of the loop and make it:
index = m_out.add(index, m_out.constIntPtr(use->child1()->numConstants())
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7064
> + if (target->op() == PhantomNewArrayBuffer)
> + length = m_out.constIntPtr(target->numConstants());
I think you can just add this to "numNonSpreadParameters". You may want to
rename that variable to something like "staticArgumentCount" or something
similar.
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7219
> + value =
bitwise_cast<int64_t>(data[i].asNumber());
ditto this is wrong. Please add tests.
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7905
> + if (target->op() == PhantomNewArrayBuffer) {
> +
spreadLengths.append(m_out.constInt32(target->numConstants()));
> + return;
This could be considered as part of numberOfStaticArguments.
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7953
> + if (hasDouble(target->indexingType()))
> + value = bitwise_cast<int64_t>(data[i].asNumber());
Ditto. This is wrong. Please add a test
> Source/WTF/wtf/FixedPointCombinator.h:33
> +struct FixedPointCombinator {
I'm not really a fan of this name. There is noting requiring Function to reach
a fixed point.
Is there a reason you chose this name?
I think a better name would have something referencing recursion in it.
More information about the webkit-reviews
mailing list