[Webkit-unassigned] [Bug 162124] op_get_by_id_with_this should use inline caching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162124

--- Comment #59 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 297746
  --> https://bugs.webkit.org/attachment.cgi?id=297746
Patch Updated

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

This patch looks pretty good. Here are some comments on various bugs I see.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
> +                GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;

Is this correct? Does DOMJIT expect the property holder or the receiver? This is a good question for Yusuke.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1154
> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;

Do you have tests for this?

> 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)

These could be JSValueRegs.

> 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));

You should assert here that both tags are not invalid.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4368
> +            JSValueOperand base(this, node->child1());
> +            JSValueOperand thisValue(this, node->child2());

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.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4355
> +            JSValueOperand base(this, node->child1());
> +            GPRReg baseGPR = base.gpr();
> +            JSValueOperand thisValue(this, node->child2());
> +            GPRReg thisValueGPR = thisValue.gpr();

Same comment here as in 32 bit implementation.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:695
> +    Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT0, regT1, ident->impl());

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.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:704
> +    Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT1, regT0, regT4, regT3, ident->impl());

ditto here.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170123/108eeb7e/attachment-0001.html>