<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - op_get_by_id_with_this should use inline caching"
href="https://bugs.webkit.org/show_bug.cgi?id=162124#c59">Comment # 59</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - op_get_by_id_with_this should use inline caching"
href="https://bugs.webkit.org/show_bug.cgi?id=162124">bug 162124</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=297746&action=diff" name="attach_297746" title="Patch Updated">attachment 297746</a> <a href="attachment.cgi?id=297746&action=edit" title="Patch Updated">[details]</a></span>
Patch Updated
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=297746&action=review">https://bugs.webkit.org/attachment.cgi?id=297746&action=review</a>
This patch looks pretty good. Here are some comments on various bugs I see.
<span class="quote">> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
> + GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;</span >
Is this correct? Does DOMJIT expect the property holder or the receiver? This is a good question for Yusuke.
<span class="quote">> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1154
> + GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;</span >
Do you have tests for this?
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2080
> + JITCompiler::Call callOperation(J_JITOperation_ESsiJJI operation, JSValueRegs result, StructureStubInfo* stubInfo, GPRReg arg1Tag, GPRReg arg1Payload, GPRReg arg2Tag, GPRReg arg2Payload, UniquedStringImpl* uid)</span >
These could be JSValueRegs.
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:288
> + slowPath = slowPathCall(
> + slowCases, this, operationGetByIdWithThisOptimize,
> + JSValueRegs(resultTagGPR, resultPayloadGPR), gen.stubInfo(), JSValueRegs(baseTagGPROrNone, basePayloadGPR), JSValueRegs(thisTagGPR, thisPayloadGPR), identifierUID(identifierNumber));</span >
You should assert here that both tags are not invalid.
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4368
> + JSValueOperand base(this, node->child1());
> + JSValueOperand thisValue(this, node->child2());</span >
This is wrong. If either base or this here are CellUse, you're failing to speculate, which is incorrect. One way to solve this is to only allow to use kind states for this node:
1 && 2 are both CellUse
1 && 2 are both Untyped use.
You could ensure this inside the FixupPhase. Otherwise, you need to handle all 4 possibilities.
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4355
> + JSValueOperand base(this, node->child1());
> + GPRReg baseGPR = base.gpr();
> + JSValueOperand thisValue(this, node->child2());
> + GPRReg thisValueGPR = thisValue.gpr();</span >
Same comment here as in 32 bit implementation.
<span class="quote">> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:695
> + Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT0, regT1, ident->impl());</span >
It seems like given your fast path code, these registers aren't guaranteed to be materialized to the values you want. Maybe load all values on the fast path before emitting a branch? Please add a test for this as it should probably be an insta-crash.
<span class="quote">> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:704
> + Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT1, regT0, regT4, regT3, ident->impl());</span >
ditto here.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>