Bug 182792 - [FTL] Support HasIndexedProperty for ArrayStorage and SlowPutArrayStorage
Summary: [FTL] Support HasIndexedProperty for ArrayStorage and SlowPutArrayStorage
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 06:16 PST by Yusuke Suzuki
Modified: 2018-02-22 22:42 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.70 KB, patch)
2018-02-14 06:17 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.72 KB, patch)
2018-02-19 23:16 PST, Yusuke Suzuki
no flags 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 06:16:36 PST
[FTL] Support HasIndexedProperty for ArrayStorage
Comment 1 Yusuke Suzuki 2018-02-14 06:17:38 PST
Created attachment 333791 [details]
Patch
Comment 2 Saam Barati 2018-02-19 19:02:56 PST
Comment on attachment 333791 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [FTL] Support HasIndexedProperty for ArrayStorage

And SlowPutArrayStorage.

> Source/JavaScriptCore/ChangeLog:8
> +        This patch adds HasIndexedProperty for ArrayStorage in FTL.

And SlowPutArrayStorage.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9781
> +                m_out.jump(checkHole);

This needs to do a speculation check that it's not out of bounds. Seems like this should be testable via crashing on some OOB read. You also won't need a "checkHole" dedicated block if it's in bounds, you can just use the incoming block we're emitting code in.
Comment 3 Yusuke Suzuki 2018-02-19 23:06:20 PST
Comment on attachment 333791 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9781
>> +                m_out.jump(checkHole);
> 
> This needs to do a speculation check that it's not out of bounds. Seems like this should be testable via crashing on some OOB read. You also won't need a "checkHole" dedicated block if it's in bounds, you can just use the incoming block we're emitting code in.

As is the same to GetByVal etc., HasIndexedProperty also has special lowering rule in SSA lowering phase, which emits CheckInBounds.
We do not need to perform out-of-bounds speculation here since preceeding CheckInBounds does this. For ArrayStorage/SlowPutArrayStorage, we emit GetVectorLength and CheckInBounds instead of GetArrayLength and CheckInBounds. So it is correctly handled.

For checkHole case, I'll update the patch to drop checkHole BB for InBounds case.
Comment 4 Yusuke Suzuki 2018-02-19 23:16:55 PST
Created attachment 334244 [details]
Patch
Comment 5 Mark Lam 2018-02-22 20:47:58 PST
Comment on attachment 334244 [details]
Patch

r=me
Comment 6 Yusuke Suzuki 2018-02-22 22:18:23 PST
Comment on attachment 334244 [details]
Patch

Thanks!
Comment 7 WebKit Commit Bot 2018-02-22 22:41:54 PST
Comment on attachment 334244 [details]
Patch

Clearing flags on attachment: 334244

Committed r228943: <https://trac.webkit.org/changeset/228943>
Comment 8 WebKit Commit Bot 2018-02-22 22:41:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-02-22 22:42:25 PST
<rdar://problem/37816653>