Bug 182782 – [FTL] Support ArrayPush for ArrayStorage

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[FTL] Support ArrayPush for ArrayStorage

[Yusuke Suzuki]

Created attachment 333776 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 333776 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4478
> +        case Array::ArrayStorage: {

ArrayStorage case is similar to Int32/Contiguous/Double. But it has various slightly different things.
For example, ArrayStorage_vector has offset while the other arrays's storage does not have offset.
And we should check largestPositiveInt32Length.
In addition, we need to increment ArrayStorage_numValuesInVector.

So, we just create the completely different path for ArrayStorage in this patch.

[Yusuke Suzuki]

Created attachment 333782 [details]
Patch

[Yusuke Suzuki]

Created attachment 333785 [details]
Patch

[Saam Barati]

Comment on attachment 333785 [details]
Patch

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

r=me

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4478
> +        case Array::ArrayStorage: {

Is SlowPutArrayStorage not possible?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4488
> +                LValue prevLength = m_out.load32(storage, m_heaps.Butterfly_publicLength);
> +                // Refuse to handle bizarre lengths.
> +                speculate(Uncountable, noValue(), nullptr, m_out.above(prevLength, m_out.constInt32(largestPositiveInt32Length)));

You can move this code above the if since you do it below too.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4497
> +                    unsure(slowPath), unsure(fastPath));

You can probably use likely/unlikely here

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4535
> +            m_out.branch(beyondVectorLength, unsure(slowPath), unsure(fastPath));

you can probably use likely/unlikely here.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4563
> +            m_out.branch(beyondVectorLength, unsure(slowCallPath), unsure(continuation));

could probably use likely/unlikely here.

[Yusuke Suzuki]

Comment on attachment 333785 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4478
>> +        case Array::ArrayStorage: {
> 
> Is SlowPutArrayStorage not possible?

Currently we do not support SlowPutArrayStorage for ArrayPush/ArrayPop (in DFGByteCodeParser).
And we cannot extend this support for SlowPutArrayStorage mechanically since ArrayPush for SlowPutArrayStorage can be reentrant while the other cases are not... so use of scratch buffer should be careful.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4488
>> +                speculate(Uncountable, noValue(), nullptr, m_out.above(prevLength, m_out.constInt32(largestPositiveInt32Length)));
> 
> You can move this code above the if since you do it below too.

Moved.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4497
>> +                    unsure(slowPath), unsure(fastPath));
> 
> You can probably use likely/unlikely here

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4535
>> +            m_out.branch(beyondVectorLength, unsure(slowPath), unsure(fastPath));
> 
> you can probably use likely/unlikely here.

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4563
>> +            m_out.branch(beyondVectorLength, unsure(slowCallPath), unsure(continuation));
> 
> could probably use likely/unlikely here.

Fixed.

[Yusuke Suzuki]

Committed r228728: <https://trac.webkit.org/changeset/228728>

[Radar WebKit Bug Importer]

<rdar://problem/37696911>