[webkit-reviews] review granted: [Bug 175823] [DFG] Support ArrayPush with multiple args : [Attachment 321778] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 175823: [DFG] Support ArrayPush with multiple args
https://bugs.webkit.org/show_bug.cgi?id=175823

Attachment 321778: Patch

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




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

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

r=me

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1039
> +	       // and ArrayPush uses the edge as known typed edge. In this way,
ArrayPush do not need to perform type checks.

In this way => Therefore

> Source/JavaScriptCore/dfg/DFGOperations.cpp:899
> +    ASSERT(array->indexingType() == ArrayWithInt32 || array->indexingType()
== ArrayWithContiguous || array->indexingType() == ArrayWithArrayStorage);

I'd vote for:
RELEASE_ASSERT(!shouldUseSlowPut(array->indexingType())

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8088
> +	   // Refuse to handle bizarre lengths.
> +	   speculationCheck(Uncountable, JSValueRegs(), 0,
m_jit.branch32(MacroAssembler::Above, storageLengthGPR,
TrustedImm32(bizarreLength)));

Let's name this something better, like largestPositiveInt32Length or something
more descriptive. "bizarre" is not a descriptive name. I'd also make the
comment state the reasoning: that DFG AI says this node results in Int32

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4162
> +	       ValueFromBlock fastResult =
m_out.anchor(m_out.baseIndex(storage, m_out.zeroExtPtr(prevLength),
ScaleEight));

let's call these fastBufferResult and slowBufferResult

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4209
> +	       setJSValue(m_out.phi(Int64, fastCallResult, slowCallResult));

and then you can just name these fastResult and slowResult since
"fastCallResult" isn't from a call