[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