Bug 182731 – [FTL] Add Arrayify for ArrayStorage and SlowPutArrayStorage

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 333702 [details]
Patch

[Yusuke Suzuki]

Created attachment 333713 [details]
Patch

[Yusuke Suzuki]

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.

[Saam Barati]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 333775 [details]
Patch

[Yusuke Suzuki]

Ping?

[Saam Barati]

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"

[Saam Barati]

(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.

[Saam Barati]

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).

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r228726: <https://trac.webkit.org/changeset/228726>

[Radar WebKit Bug Importer]

<rdar://problem/37695589>

[Yusuke Suzuki]

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()`.