[webkit-reviews] review granted: [Bug 182965] [FTL] Support PutByVal(ArrayStorage/SlowPutArrayStorage) : [Attachment 334592] 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 182965: [FTL] Support PutByVal(ArrayStorage/SlowPutArrayStorage)
https://bugs.webkit.org/show_bug.cgi?id=182965

Attachment 334592: Patch

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




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

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

r=me

> Source/JavaScriptCore/dfg/DFGOperations.cpp:810
> +void JIT_OPERATION
operationPutDoubleByValDirectBeyondArrayBoundsStrict(ExecState* exec, JSObject*
array, int32_t index, double value)

Is this actually guaranteed to be a JSArray* here? If so, I'd give it that
type. Otherwise, perhaps just name the variable "object".

> Source/JavaScriptCore/dfg/DFGOperations.cpp:826
> +void JIT_OPERATION
operationPutDoubleByValDirectBeyondArrayBoundsNonStrict(ExecState* exec,
JSObject* array, int32_t index, double value)

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2084
> +		   m_jit.codeBlock()->isStrictMode()
> +		       ? (node->op() == PutByValDirect ?
operationPutDoubleByValDirectBeyondArrayBoundsStrict :
operationPutDoubleByValBeyondArrayBoundsStrict)
> +		       : (node->op() == PutByValDirect ?
operationPutDoubleByValDirectBeyondArrayBoundsNonStrict :
operationPutDoubleByValBeyondArrayBoundsNonStrict),

Was this doing the wrong thing before?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4274
> +	       LBasicBlock notHoleCase = m_out.newBlock();

nit: This isn't really "notHoleCase" since the hole case may jump here. I'd
call this "doStoreCase" or something like that

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4300
> +			   index, m_out.load32NonNegative(storage,
m_heaps.Butterfly_publicLength)),

Is it worth defining an ArrayStorage_publicLength just in case someone changes
the memory layout to not be the same as Butterfly?