[webkit-reviews] review denied: [Bug 128352] ReallocatePropertyStorage in FTL : [Attachment 223751] ReallocatePropertyStorage in FTL patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 128352: ReallocatePropertyStorage in FTL
https://bugs.webkit.org/show_bug.cgi?id=128352

Attachment 223751: ReallocatePropertyStorage in FTL patch
https://bugs.webkit.org/attachment.cgi?id=223751&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223751&action=review


> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2692
> +	       LBasicBlock failPath = FTL_NEW_BLOCK(m_out,
("ReallocatePropertyStorage failure"));
> +	       LBasicBlock continuation = FTL_NEW_BLOCK(m_out,
("ReallocatePropertyStorage continuation"));
> +	       
> +	       m_out.branch(m_out.isNull(result) , failPath , continuation);
> +	       
> +	       m_out.appendTo(failPath, continuation);
> +	       m_out.crash();
> +	       
> +	       m_out.appendTo(continuation);

Why is this here?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2727
> +	       ptrdiff_t off  = offsetRelativeToBase(prop.offset);
> +	       LValue loaded = 
> +		   m_out.loadPtr(m_out.address(m_heaps.properties[ident],
oldStorage, -off));
> +	       m_out.storePtr(loaded, m_out.address(m_heaps.properties[ident] ,
result, -off));

I think that the negation ("-off") is wrong.  offsetRelativeToBase() should
already return an appropriately negated value.	Also, you shouldn't copy the
inline properties, but this appears to do that.  Also, 'item' here shouldn't be
used as an index into m_storageAccessData.  m_storageAccessData doesn't use
indexes into property storage as an index.  We just add an entry into
m_storageAccessData whenever we add a node that needs it; it has nothing to do
with the properties of any particular type.