[webkit-reviews] review denied: [Bug 128850] GetMyArgumentByVal in FTL : [Attachment 224911] Added GetMyArgumentByVal to FTL

[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 224911: Added GetMyArgumentByVal to FTL
https://bugs.webkit.org/attachment.cgi?id=224911&action=review

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


This looks good to me, but I would fix the indentation/style goofs.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2044
> +		LValue property = lowInt32(m_node->child1());

Weird indentation.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2045
> +	   CodeOrigin& codeLocation = m_node->origin.semantic;

I wouldn't use a reference here.  There's no point since it's a small struct
and probably copying it from the heap is faster than having a pointer to it. 
It's also somewhat safer in general.  I mean, in this particular case, nobody
will delete the thing that m_node points to, but in general I like to
pass-by-value when I can because it means I don't have to worry about object
lifecycles as much.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2050
> +	   if
(!isEmptySpeculation(m_state.variables().operand(m_graph.argumentsRegisterFor(c
odeLocation)).m_type))
> +	       speculate(ArgumentsEscaped, noValue(), 0,
> +		  
m_out.notNull(m_out.loadPtr(addressFor(m_graph.machineArgumentsRegisterFor(code
Location)))));

Multi-line control clauses need { }.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2059
> +	   if (inlineCallFrame)
> +	       speculate(Uncountable, noValue(), 0, 
> +		   m_out.aboveOrEqual(property,
> +		       m_out.constInt32(inlineCallFrame->arguments.size() -
1)));
> +	   else 
> +	       speculate(Uncountable, noValue(), 0, 
> +		   m_out.aboveOrEqual(m_out.add(property, m_out.constInt32(1)),

> +		      
m_out.load32NonNegative(payloadFor(JSStack::ArgumentCount))));

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2063
> +	       RELEASE_ASSERT(false); // TODO

RELEASE_ASSERT_NOT_REACHED().  Good form would be to file a bug on bugzilla for
the part that needs to be done.  It can be a super terse bug, like title = "FTL
GetArgumentByVal should handle the slowArguments case", and description = "..."
(I use that to have a non-empty string in the description to make bugzilla
happy.)

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2070
> +	  
setJSValue(m_out.load64(m_out.baseIndex(m_heaps.variables.atAnyIndex(),
> +	       inlineCallFrame ?
addressFor(inlineCallFrame->arguments[0].virtualRegister()).value() :
m_callFrame,
> +	       m_out.signExt(property, m_out.intPtr), ScaleEight, 
> +	       offsetOfArguments(inlineCallFrame))));
> +    }

This looks suspicious in case of inlined code.	You're first getting the
addressFor() arguments[0], and then you're putting in an offset which again
calculates the offset of arguments[1].	Both of these calculations will contain
almost the same offset.  This seems wrong.  It'll work for not-inlined code
since you use m_callFrame as a base and then put in the argumentsOffset of the
zero'th argument.  But for inlined code you're adding something like that
twice.	Also, inlineCallFrame->arguments[0] could be unset, leading to a crash.


I think that you should try to rearchitect this so that you're first computing
the address of the zero'th argument and then passing that as the base of
baseIndex, and then you'll use zero as the offset (i.e. you won't pass the
offset).  The helper should therefore be addressOfArguments rather than
offsetOfArguments.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2081
> +    int offsetOfArguments(InlineCallFrame* inlineCallFrame)
> +    {
> +	   if (!inlineCallFrame)
> +	       return CallFrame::argumentOffset(0) * sizeof(Register);
> +	   if (inlineCallFrame->arguments.size() <= 1)
> +	       return 0;
> +	   ValueRecovery recovery = inlineCallFrame->arguments[1];
> +	   RELEASE_ASSERT(recovery.technique() == DisplacedInJSStack);
> +	   return recovery.virtualRegister().offset() * sizeof(Register);
> +    }

Move this below all of the compileBlahBlah method definitions.	Also, I think
the convention we usually use is that we pass around VirtualRegisters right up
until the bitter end.  So, you could call this registerOfBaseArgument or
something.  It would be the same as what you did except it would return a
VirtualRegister and it wouldn't be a multiple of sizeof(Register).  But, I
think it would be even better if you just made a helper called
addressOfArguments that returns a LValue that is the address of the zero'th
argument.