[webkit-reviews] review denied: [Bug 214861] [JSC] support op_get_private_name in DFG : [Attachment 407619] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 214861: [JSC] support op_get_private_name in DFG
https://bugs.webkit.org/show_bug.cgi?id=214861

Attachment 407619: Patch

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




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

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

Some comments below. r- because of some bugs I found and the lack of ICs for
the get_by_val style GetPrivateName

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:407
> +    case AccessType::GetPrivateName:

Should we add a new enum value GetPrivateNameById?

It seems wrong we didn't handle the get_by_val style GetPrivateName here
before.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4731
> +void ByteCodeParser::handleGetPrivateName(

nit: maybe name this "handleGetPrivateNameById"?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4733
> +    // Attempt to reduce the set of things in the GetByStatus.

can we make a helper for this  loop below since it's the same as the code in
get by id helper?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4778
> +	       GetByOffsetMethod method = variant.conditionSet().isEmpty()
> +		   ? GetByOffsetMethod::load(variant.offset())
> +		   : planLoad(variant.conditionSet());

I think the conditionSet should always be empty, since we're doing a self load
for get private name.

Maybe we should assert this?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4780
> +	       if (!method) {

this might not be needed after that change

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2766
> +	   case GetPrivateName:
> +	   case GetPrivateNameById:

we should implement these. We want to speculate cell for the base. And maybe
speculate symbol for the property in GetPrivateName

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3483
> +    callOperation(operationGetPrivateName, resultRegs,
TrustedImmPtr::weakPointer(m_graph,
m_graph.globalObjectFor(node->origin.semantic)), TrustedImmPtr(nullptr),
baseGPR, propertyGPR);

Let's make this do the same IC as baseline. And let's add some microbenchmarks
for it.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3931
> +    void compileGetPrivateName()

ditto about making this do the same IC we do in baseline

> Source/JavaScriptCore/jit/JITOperations.cpp:2281
> +    if (stubInfo)
> +	   stubInfo->tookSlowPath = true;

we can revert this part when we start doing the IC

> Source/JavaScriptCore/jit/JITOperations.cpp:2317
> +EncodedJSValue JIT_OPERATION
operationGetPrivateNameByIdGeneric(JSGlobalObject* globalObject, EncodedJSValue
base, uintptr_t rawCacheableIdentifier)

since operationGetPrivateNameByIdOptimize can be rewatched to call here, we
will end up with bad things happening because of the different function
signatures.

This needs to match operationGetPrivateNameByIdOptimize signature, and to mark
the StructureStubInfo' "takes slow path" bit.

Let's add a test for this, since it the current implementation should result in
a crash.