Bug 234387 – [JSC][ARMv7] Minor code size improvements

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geza Lore]

[JSC][ARMv7] Minor code size improvements

[Geza Lore]

Created attachment 447342 [details]
Patch

[Geza Lore]

Created attachment 447343 [details]
Patch

[Geza Lore]

Created attachment 447344 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 447344 [details]
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();

[Geza Lore]

Created attachment 447589 [details]
Fix review feedback

[Geza Lore]

Comment on attachment 447344 [details]
Patch

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

Thanks these are very helpful. Fixed all.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2263
>> +            m_jit.storeValue(value.jsValueRegs(), JITCompiler::addressFor(node->machineLocal()));
> 
> First define JSValueRegs valueRegs = value.jsValueRegs();

Not sure this is necessary, most other cases don't do this.

[Yusuke Suzuki]

Comment on attachment 447589 [details]
Fix review feedback

r=me

[EWS]

Committed r287301 (245453@main): <https://commits.webkit.org/245453@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447589 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86757902>