[webkit-reviews] review denied: [Bug 234387] [JSC][ARMv7] Minor code size improvements : [Attachment 447344] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Geza Lore <glore at igalia.com>'s
request for review:
Bug 234387: [JSC][ARMv7] Minor code size improvements
https://bugs.webkit.org/show_bug.cgi?id=234387

Attachment 447344: Patch

https://bugs.webkit.org/attachment.cgi?id=447344&action=review




--- Comment #4 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 447344
  --> https://bugs.webkit.org/attachment.cgi?id=447344
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447344&action=review

Nice. Commented.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:-696
> -	   GPRReg thisArgumentTagGPR = thisArgument.tagGPR();
> -	   GPRReg thisArgumentPayloadGPR = thisArgument.payloadGPR();
>	   thisArgument.use();
>	   
> -	   m_jit.store32(thisArgumentTagGPR,
JITCompiler::calleeArgumentTagSlot(0));
> -	   m_jit.store32(thisArgumentPayloadGPR,
JITCompiler::calleeArgumentPayloadSlot(0));

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:-758
> -		   GPRReg argTagGPR = arg.tagGPR();
> -		   GPRReg argPayloadGPR = arg.payloadGPR();
>		   use(argEdge);
> -	       
> -		   m_jit.store32(argTagGPR, m_jit.calleeArgumentTagSlot(i));
> -		   m_jit.store32(argPayloadGPR,
m_jit.calleeArgumentPayloadSlot(i));

This is not correct. Need to call arg.jsValueRegs() first before calling use()
IIRC.
So, please follow the original way of the code, do not pass `arg.jsValueRegs()`
arg directly, and instead, first define the following.

JSValueRegs argsRegs = args.jsValueRegs();

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:763
>	   calleePayloadGPR = callee.payloadGPR();

Let's follow the protocol the other DFG part uses.

First, define

JSValueRegs calleeRegs = callee.jsValueRegs();

before calling use.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2193
>	       GPRTemporary result(this);
>	       GPRTemporary tag(this);

How about using JSValueRegsTemporary?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2194
> +	       m_jit.loadValue(JITCompiler::addressFor(node->machineLocal()),
JSValueRegs { tag.gpr(), result.gpr() });

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2263
> -	       m_jit.store32(value.payloadGPR(),
JITCompiler::payloadFor(node->machineLocal()));
> -	       m_jit.store32(value.tagGPR(),
JITCompiler::tagFor(node->machineLocal()));
> +	       m_jit.storeValue(value.jsValueRegs(),
JITCompiler::addressFor(node->machineLocal()));

First define JSValueRegs valueRegs = value.jsValueRegs();