[Webkit-unassigned] [Bug 128626] Lowering of CheckArray in FTL not supported for all types.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=128626


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #223915|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Filip Pizlo <fpizlo at apple.com>  2014-02-11 17:06:48 PST ---
(From update of attachment 223915)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.