[webkit-reviews] review denied: [Bug 128850] GetMyArgumentByVal in FTL : [Attachment 224567] Added GetMyArgumentByVal to FTL and fixed compileGetMyArgumentsLength

[bugzilla-daemon at webkit.org]

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

Attachment 224567: Added GetMyArgumentByVal to FTL and fixed
compileGetMyArgumentsLength
https://bugs.webkit.org/attachment.cgi?id=224567&action=review

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


> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1816
> +	   if (codeLocation.inlineCallFrame) {

You don't need enclosing { } for one-line control clauses.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1819
> +	       TypedPointer reg = addressFor(JSStack::ArgumentCount); 

payloadFor()?  I think your code works for now because the payload is the low
32 bits.  But, it would break if we ever went big endian.  (Not that any
sensible person would ever use big endian; little endian is way better.)

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2062
> +		   JSStack::ThisArgument * sizeof(Register) +
sizeof(Register))));

You don't need this extra offset thingy, the "JSStack::ThisArgument *
sizeof...".  In fact, having that makes the code wrong.

Have you run-javascriptcore-tests btw? ;-)

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2069
> +		    
m_out.isNull(m_out.loadPtr(m_out.address(m_heaps.variables.atAnyIndex(),
m_callFrame, Arguments::offsetOfSlowArgumentData()))));

Shouldn't you be exiting if the slow argument data is non-null, rather than
when it's null?  Also, I don't understand what you're loading here.  You seem
to be loading the frame pointer from the stack, which is indeed never null, and
also has nothing to do with slow arguments.