Bug 128626 – Lowering of CheckArray in FTL not supported for all types.

• Status: NEW
• Component: JavaScriptCore

[Matthew Mirman]

patch forthcoming.

[Matthew Mirman]

Created attachment 223905 [details]
Added more functionality to CheckArray

[Filip Pizlo]

Comment on attachment 223905 [details]
Added more functionality to CheckArray

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:-1756
> -        LValue cell = lowCell(edge);

Why remove this here?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1776
> +            LValue cell = lowCell(edge);            

And move it here?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1792
> +        LValue cell = lowCell(m_node->child1());

... and then have to do it again here?  Seems like it was happiest at the top.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1794
> +        LValue obj = m_out.loadPtr(m_out.address(cell , m_heaps.JSCell_structure)); 

I wouldn't call this "obj".  I would call it "structure".

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1798
> +                , m_out.constIntPtr(expectedClassInfo)));

Weird indentation and comma placement.  Put the comma at the end of the previous line.

[Matthew Mirman]

Created attachment 223910 [details]
Added more functionality to CheckArray

[Filip Pizlo]

Comment on attachment 223910 [details]
Added more functionality to CheckArray

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4715
> -        case Array::Contiguous: {
> +        case Array::Contiguous: 
> +        case Array::ArrayStorage: 
> +        case Array::SlowPutArrayStorage: {
>              LValue indexingType = m_out.load8(
>                  m_out.loadPtr(cell, m_heaps.JSCell_structure),
>                  m_heaps.Structure_indexingType);

I don't think that you can just add ArrayStorage and SlowPutArrayStorage here.  See SpeculativeJIT::jumpSlowForUnwantedArrayMode().

[Matthew Mirman]

Created attachment 223914 [details]
Added more functionality to CheckArray

[Matthew Mirman]

Created attachment 223915 [details]
Added more functionality to CheckArray

There was a bug in the previous patch file.

[Filip Pizlo]

Comment on attachment 223915 [details]
Added more functionality to CheckArray

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1774
> +        case Array::ArrayStorage: 
> +        case Array::SlowPutArrayStorage: {

Since you're claiming to handle ArrayStorage and SlowPutArraySTorage but isArrayType() doesn't have cases for either, won't this generate bad code for ArrayStorage and SlowPutArrayStorage?  In particular, isArrayType() assumes that anything that isn't Array::Int32, Array::Double, Array::Contiguous can be checked using the class info.  That's wrong.

But even more fundamentally, why aren't all of your changes in isArrayType()?  For example, for typed arrays, your change just adds noise here in CheckArray.  isArrayType() would have done the right thing for those.  So, you could just teach isArrayType() to do the right thing for Arguments and ArrayStorage/SlowPutArrayStorage.

[Matthew Mirman]

(In reply to comment #7)
> (From update of attachment 223915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223915&action=review
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1774
> > +        case Array::ArrayStorage: 
> > +        case Array::SlowPutArrayStorage: {
> 
> Since you're claiming to handle ArrayStorage and SlowPutArraySTorage but isArrayType() doesn't have cases for either, won't this generate bad code for ArrayStorage and SlowPutArrayStorage?  In particular, isArrayType() assumes that anything that isn't Array::Int32, Array::Double, Array::Contiguous can be checked using the class info.  That's wrong.
> 
> But even more fundamentally, why aren't all of your changes in isArrayType()?  For example, for typed arrays, your change just adds noise here in CheckArray.  isArrayType() would have done the right thing for those.  So, you could just teach isArrayType() to do the right thing for Arguments and ArrayStorage/SlowPutArrayStorage.

You're right, I got ahead of myself with the thing I was looking at after this.