Bug 182731 - [FTL] Add Arrayify for ArrayStorage and SlowPutArrayStorage
Summary: [FTL] Add Arrayify for ArrayStorage and SlowPutArrayStorage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 182625
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-13 09:31 PST by Yusuke Suzuki
Modified: 2018-02-19 20:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.85 KB, patch)
2018-02-13 10:38 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.82 KB, patch)
2018-02-13 12:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.59 KB, patch)
2018-02-14 01:25 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-13 09:31:22 PST
...
Comment 1 Yusuke Suzuki 2018-02-13 10:38:54 PST
Created attachment 333702 [details]
Patch
Comment 2 Yusuke Suzuki 2018-02-13 12:45:06 PST
Created attachment 333713 [details]
Patch
Comment 3 Yusuke Suzuki 2018-02-13 12:48:41 PST
Comment on attachment 333713 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-791
> -        m_jit.and32(TrustedImm32(IndexingShapeMask), tempGPR);

Old DFG implementation does not check Array::NonArray/Array::OriginalNonArray well (it does not look into IsArray).
This patch also fixes it.
Comment 4 Saam Barati 2018-02-13 14:52:38 PST
Comment on attachment 333713 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:780
> +                    TrustedImm32(SlowPutArrayStorageShape - ArrayStorageShape)));

This math doesn't look right to me:
input indexing type = i = 0x0c (NonArrayWithSlowPutArrayStorage)
i - (0x0A | 1) = 1
1 is not above (0x0c - 0x0a) = 2
Comment 5 Yusuke Suzuki 2018-02-14 01:25:06 PST
Comment on attachment 333713 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:780
>> +                    TrustedImm32(SlowPutArrayStorageShape - ArrayStorageShape)));
> 
> This math doesn't look right to me:
> input indexing type = i = 0x0c (NonArrayWithSlowPutArrayStorage)
> i - (0x0A | 1) = 1
> 1 is not above (0x0c - 0x0a) = 2

Oops, I thought IsArray is higher than SlowPutArrayStorageShape. Fixed.
Comment 6 Yusuke Suzuki 2018-02-14 01:25:52 PST
Created attachment 333775 [details]
Patch
Comment 7 Yusuke Suzuki 2018-02-17 22:16:26 PST
Ping?
Comment 8 Saam Barati 2018-02-19 18:36:23 PST
Comment on attachment 333775 [details]
Patch

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

r=me with some FTL comments

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14590
> +            LValue shapeResult = m_out.belowOrEqual(

nit: I'd maybe call this something like "isExpectedShape" or "isAnArrayStorageShape" or something similar.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14595
> +            m_out.branch(shapeResult, usually(checkCase), usually(falseCase));

these are both usually, that's wrong. The "falseCase" should be "unlikely"
Comment 9 Saam Barati 2018-02-19 18:37:15 PST
(In reply to Saam Barati from comment #8)
> Comment on attachment 333775 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333775&action=review
> 
> r=me with some FTL comments
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14590
> > +            LValue shapeResult = m_out.belowOrEqual(
> 
> nit: I'd maybe call this something like "isExpectedShape" or
> "isAnArrayStorageShape" or something similar.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14595
> > +            m_out.branch(shapeResult, usually(checkCase), usually(falseCase));
> 
> these are both usually, that's wrong. The "falseCase" should be "unlikely"

Oops, I has two reviews going in two different windows. Going to comment more.
Comment 10 Saam Barati 2018-02-19 18:38:42 PST
Comment on attachment 333775 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14595
>> +            m_out.branch(shapeResult, usually(checkCase), usually(falseCase));
> 
> these are both usually, that's wrong. The "falseCase" should be "unlikely"

Ignore be saying which one should be likely/unlikely. I'll let you decide that.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14606
> +                    usually(trueCase), usually(falseCase));

You use usually twice here again.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14613
> +                    usually(trueCase), usually(falseCase));

and here.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14627
> +            m_out.appendTo(falseCase, continuation);
> +            ValueFromBlock falseValue = m_out.anchor(m_out.booleanFalse);
> +            m_out.jump(continuation);

You don't need a basic block to do this. You can just anchor this in the initial basic block and remove this. (Or you can remove the true block and do something similar there).
Comment 11 Yusuke Suzuki 2018-02-19 19:58:15 PST
Comment on attachment 333775 [details]
Patch

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

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14590
>>> +            LValue shapeResult = m_out.belowOrEqual(
>> 
>> nit: I'd maybe call this something like "isExpectedShape" or "isAnArrayStorageShape" or something similar.
> 
> Oops, I has two reviews going in two different windows. Going to comment more.

OK, I'll use `isAnArrayStorageShape`.

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14595
>>> +            m_out.branch(shapeResult, usually(checkCase), usually(falseCase));
>> 
>> these are both usually, that's wrong. The "falseCase" should be "unlikely"
> 
> Ignore be saying which one should be likely/unlikely. I'll let you decide that.

I think using `usually` is ok here since the condition produced by this function would be used for both cases.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14627
>> +            m_out.jump(continuation);
> 
> You don't need a basic block to do this. You can just anchor this in the initial basic block and remove this. (Or you can remove the true block and do something similar there).

Nice, I've moved falseValue anchor to the initial basic block, and remove this `falseCase` BB.
Comment 12 Yusuke Suzuki 2018-02-19 20:00:22 PST
Committed r228726: <https://trac.webkit.org/changeset/228726>
Comment 13 Radar WebKit Bug Importer 2018-02-19 20:01:31 PST
<rdar://problem/37695589>
Comment 14 Yusuke Suzuki 2018-02-19 20:50:32 PST
Comment on attachment 333775 [details]
Patch

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

>>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14595
>>>> +            m_out.branch(shapeResult, usually(checkCase), usually(falseCase));
>>> 
>>> these are both usually, that's wrong. The "falseCase" should be "unlikely"
>> 
>> Ignore be saying which one should be likely/unlikely. I'll let you decide that.
> 
> I think using `usually` is ok here since the condition produced by this function would be used for both cases.

Oops, I misunderstood your comment. I'll change them to `unsure()`.