Bug 182782 - [FTL] Support ArrayPush for ArrayStorage
Summary: [FTL] Support ArrayPush for ArrayStorage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-14 02:14 PST by Yusuke Suzuki
Modified: 2018-02-19 21:03 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.21 KB, patch)
2018-02-14 02:14 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.69 KB, patch)
2018-02-14 03:37 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.91 KB, patch)
2018-02-14 03:59 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-02-14 02:14:18 PST
[FTL] Support ArrayPush for ArrayStorage
Comment 1 Yusuke Suzuki 2018-02-14 02:14:58 PST
Created attachment 333776 [details]
Patch
Comment 2 Yusuke Suzuki 2018-02-14 02:17:51 PST
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.
Comment 3 Yusuke Suzuki 2018-02-14 03:37:05 PST
Created attachment 333782 [details]
Patch
Comment 4 Yusuke Suzuki 2018-02-14 03:59:22 PST
Created attachment 333785 [details]
Patch
Comment 5 Saam Barati 2018-02-19 19:18:28 PST
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.
Comment 6 Yusuke Suzuki 2018-02-19 20:55:10 PST
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.
Comment 7 Yusuke Suzuki 2018-02-19 21:02:02 PST
Committed r228728: <https://trac.webkit.org/changeset/228728>
Comment 8 Radar WebKit Bug Importer 2018-02-19 21:03:30 PST
<rdar://problem/37696911>