<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#c39">Comment # 39</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=293804&action=diff" name="attach_293804" title="Patch">attachment 293804</a> <a href="attachment.cgi?id=293804&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=293804&action=review">https://bugs.webkit.org/attachment.cgi?id=293804&action=review</a>
<span class="quote">> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:833
> const Identifier& ident = *state.ident;</span >
Have you tested this code when we have the result/thisGPR be aliased to each other? What about when an exception is thrown and they're aliased and you're in the DFG/FTL? You should prove to yourself that these work, and also write test cases that stress them (at least for now, since register allocation may change in the future)
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4370
> + base.use();
> + thisValue.use();</span >
This is not correct, we may need these if we throw an exception. So we're not yet done using these inputs.
(I know this is done in other places, and those places are also wrong).
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4338
> + notCellList.append(m_jit.branchIfNotCell(JSValueRegs(baseGPR)));
> + notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisValueGPR)));</span >
Can you add modes to this node where we have CellUse for child1 and child2? Seems like this will be the common case, and the DFG may have already proven that these are cells.
<span class="quote">> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855
> + m_out.branch(
> + isCell(base, provenType(m_node->child1())), unsure(cellCase), unsure(notCellCase));</span >
What about child2() being a cell? Also, see above comment about having a form where we have CellUse for these things.
<span class="quote">> Source/JavaScriptCore/jit/Repatch.cpp:347
> +void repatchGetByID(ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PropertySlot& slot, StructureStubInfo& stubInfo, GetByIDKind kind, FunctionPtr repatchCallOperation)</span >
I would add a new GetByIDKind and change the appropriateGenerecGetByIdFunction instead of changing this method to take an extra parameter.</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>