[webkit-reviews] review denied: [Bug 128626] Lowering of CheckArray in FTL not supported for all types. : [Attachment 223915] Added more functionality to CheckArray

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 128626: Lowering of CheckArray in FTL not supported for all types.
https://bugs.webkit.org/show_bug.cgi?id=128626

Attachment 223915: Added more functionality to CheckArray
https://bugs.webkit.org/attachment.cgi?id=223915&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.