Bug 214861 – [JSC] support op_get_private_name in DFG and FTL

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caitlin Potter (:caitp)]

[JSC] support op_get_private_name in DFG

[Caitlin Potter (:caitp)]

Created attachment 405337 [details]
Patch

[Caitlin Potter (:caitp)]

I guess --no-ews doesn't work

[Caio Lima]

Comment on attachment 405337 [details]
Patch

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

`

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6190
> +                    handleGetById(bytecode.m_dst, prediction, base, identifier, identifierNumber, getByStatus, AccessType::GetPrivateName, nextOpcodeIndex());

There is a bug here when we use `AccessType::GetPrivateName`. This type won't be considered by `handleGetById` and it can generate a `GetByIdDirect` node when the call-site is too polymorphic. Since there is no info stored into `GetByIdDirect` about private field, the code there wil not handle semantics of missing private field correctly. This is a test case that fails:

```
//@ requireOptions("--allowUnsupportedTiers=true", "--usePrivateClassFields=true", "--maxPolymorphicAccessInliningListSize=2")
let i = 0;

class C {
    #field = this.init();

    init() {
        let arr = ["p1", "p2", "p3"];

        let key = arr[i % 3];
        this[key] = i;
        if (key === "p2")
            this["p1"] = i;

        if (key === "p3") {
            this["p1"] = i;
            this["p2"] = i;
        }

        return 'private';
    }

    getField() {
        return this.#field;
    }
}
noInline(C.prototype.getField);

let c1 = new C;
i++;
let c2 = new C;
i++;
let c3 = new C;

let arr = [c1, c2, c3];

for (; i < 10000; i++) {
    if (i > 5000) {
        assert.throws(TypeError, function() {
            C.prototype.getField.call({});
        });
    } else {
        arr[i % 3].getField();
    }
}
```

This happens because the JITGetByIdGenerator used by `GetByIdDirect` will use a slow path function with `get_by_id_direct` semantics, instead of `get_private_name`.

[Caitlin Potter (:caitp)]

Created attachment 405436 [details]
Patch

This one adds a PrivateName-specific (and limited) variant of handleGetById which produces the appropriate slow path function. I am sure that could be a maintenance headache, so the idea might need to be refined a bit. But it's something

[Caio Lima]

(In reply to Caitlin Potter (:caitp) from comment #4)
> Created attachment 405436 [details]
> Patch
> 
> This one adds a PrivateName-specific (and limited) variant of handleGetById
> which produces the appropriate slow path function. I am sure that could be a
> maintenance headache, so the idea might need to be refined a bit. But it's
> something

I think it is a bit of a shame we can't have benefits from emitting `GetByIdDirect`, like the current things for AI, constant folding, Fixup, and IC for generic paths. We probably want to add such things for `GetPrivateName` in the future and this discussion wouldn't make much sense anymore, but I'm wondering why you decided to avoid emit `GetByIdDirect` in this patch. I'm handling a similar thing into `PutPrivateName` (https://bugs.webkit.org/show_bug.cgi?id=213372) and I decided to add `PrivateFieldPutKind` into `PutByValDirect`'s OpInfo instead.

[Caitlin Potter (:caitp)]

It see(In reply to Caio Lima from comment #5)
> (In reply to Caitlin Potter (:caitp) from comment #4)
> > Created attachment 405436 [details]
> > Patch
> > 
> > This one adds a PrivateName-specific (and limited) variant of handleGetById
> > which produces the appropriate slow path function. I am sure that could be a
> > maintenance headache, so the idea might need to be refined a bit. But it's
> > something
> 
> I think it is a bit of a shame we can't have benefits from emitting
> `GetByIdDirect`, like the current things for AI, constant folding, Fixup,
> and IC for generic paths. We probably want to add such things for
> `GetPrivateName` in the future and this discussion wouldn't make much sense
> anymore, but I'm wondering why you decided to avoid emit `GetByIdDirect` in
> this patch. I'm handling a similar thing into `PutPrivateName`
> (https://bugs.webkit.org/show_bug.cgi?id=213372) and I decided to add
> `PrivateFieldPutKind` into `PutByValDirect`'s OpInfo instead.

Right now, I just want to eliminate cases where the language feature is broken. We can still add constant folding for GetPrivateName nodes, or IC in the generic case. But it's non-trivial to reuse the existing handleGetById design, it would require doing all of that work. It's not as simple as just telling it to emit GetPrivateName instead of GetByIdDirect

[Radar WebKit Bug Importer]

<rdar://problem/66503846>

[Caio Lima]

(In reply to Caitlin Potter (:caitp) from comment #6)
> It see(In reply to Caio Lima from comment #5)
> > (In reply to Caitlin Potter (:caitp) from comment #4)
> > > Created attachment 405436 [details]
> > > Patch
> > > 
> > > This one adds a PrivateName-specific (and limited) variant of handleGetById
> > > which produces the appropriate slow path function. I am sure that could be a
> > > maintenance headache, so the idea might need to be refined a bit. But it's
> > > something
> > 
> > I think it is a bit of a shame we can't have benefits from emitting
> > `GetByIdDirect`, like the current things for AI, constant folding, Fixup,
> > and IC for generic paths. We probably want to add such things for
> > `GetPrivateName` in the future and this discussion wouldn't make much sense
> > anymore, but I'm wondering why you decided to avoid emit `GetByIdDirect` in
> > this patch. I'm handling a similar thing into `PutPrivateName`
> > (https://bugs.webkit.org/show_bug.cgi?id=213372) and I decided to add
> > `PrivateFieldPutKind` into `PutByValDirect`'s OpInfo instead.
> 
> Right now, I just want to eliminate cases where the language feature is
> broken. We can still add constant folding for GetPrivateName nodes, or IC in
> the generic case. But it's non-trivial to reuse the existing handleGetById
> design, it would require doing all of that work. It's not as simple as just
> telling it to emit GetPrivateName instead of GetByIdDirect

This makes total sense. It makes more sense to add optimizations to `GetPrivateName` and leave `GetByIdDirect` out of the path.

[Caitlin Potter (:caitp)]

Created attachment 407619 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 407619 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +        DFGConstantFoldingPhase, or a GetByID IC when lowering to B3.

I'm not 100% sure the GetByID version can actually occur in real code (I sort of asked this on Slack). Without disabling (polymorphic) access inlining, I haven't been able to get the compiler to produce that path.

Perhaps, with the code visible, some smart people out there could help me figure out how to reproduce this in real world code, or otherwise suggest just not emitting the GetByID IC code in FTL.

[Saam Barati]

Comment on attachment 407619 [details]
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.

[Caitlin Potter (:caitp)]

Comment on attachment 407619 [details]
Patch

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

I've addressed these comments, thanks for the review. I am going to let the bots work on this while I craft a microbenchmark to see if using the GetByVal ICs help anything.

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

this is specifically for GetById ICs, which previously, GetPrivateName has not used

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

I'm fairly confident this is working correctly. This shares the same function signature as operationGetByIdGeneric, operationTryGetByIdGeneric, etc, and uses the same mechanisms as those to generate the slow path call.

[Caitlin Potter (:caitp)]

Created attachment 407709 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 407711 [details]
Patch

[Caio Lima]

Comment on attachment 407711 [details]
Patch

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

I think we are missing a bunch of test cases here. The I can enumerate now:

1. Case where we have constant identifier and never OSR
2. Case where we OSR due to structure check
3. Case where we compile a function considering constant ID, but then OSR due to id invalidation
4. Polymprphic call that don't OSR and also takes slow path
5. Tests that causes JITGetByIdIC repatch to unoptimized version
6. Tests that causes JITGetByValIC  repatch to unoptimized version during DFG/FTL
7. Test that takes slow path on JITGetByValIC due to id check failure on DFG/FTL
8. Tests that should throw exception when we have code `GetPrivateName`
9. Tests that should throw exception when we have code `GetPrivateNameById`

> Source/JavaScriptCore/ChangeLog:18
> +        In FTL, GetPrivateNameByID can be reduced to [Multi]GetByOffset in the

I think we can also get `GetByOffset` reduction for DFG code as well, no?

> Source/JavaScriptCore/jit/JITOperations.h:279
> +EncodedJSValue JIT_OPERATION operationGetPrivateNameByIdOptimize(JSGlobalObject*, StructureStubInfo*, EncodedJSValue, uintptr_t) WTF_INTERNAL;

Just for the record, `GetById` is repatching it's "optimize" version to `operationGetById`. Since we have `GetByKind::PrivateName` pointing to `operationGetPrivateName` we should be good there. But It would be necessary to have a test to cover that this repatch is not broken in the future.

[Caio Lima]

Comment on attachment 407711 [details]
Patch

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

> JSTests/stress/dfg-get-private-name-by-id.js:70
> +    return Object.assign({}, bases[i & bases.length]);

I think you meant to have "%" here

> JSTests/stress/dfg-get-private-name-by-id.js:94
> +  optimizeNextInvocation(test);

`test` being compiled doesn't guarantee that `getPrivate` will be compiled as well, mainly because it can't be inlined.

[Caitlin Potter (:caitp)]

Created attachment 407823 [details]
Patch

this should fix the arm/mips failures I think, new tests and microbenchmarks are still coming

[Caitlin Potter (:caitp)]

Created attachment 410149 [details]
Patch

[Caio Lima]

Comment on attachment 410149 [details]
Patch

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

I'm giving r- because I've found a bug for 32-bits (bots are also red). Overall, patch LGTM besides 32-bits issues.

> Source/JavaScriptCore/bytecode/GetByStatus.cpp:111
> +    case op_get_private_name:

I think this is something we'd like to add in future patches. DO you mind add a comment or even a FIXME to use IC information from LLInt to populate GetByStatus?

Also, can we double check that GetByIdStatus is doing the right thing for `get_private_name`? We have stronger requirements than common `get_by_[val|id]` because we need to throw exception when private field is not present in an object and never lookup prototype chain. Since we do Unset and Prototype load IC, it would be good to include ASSERTs on paths we don't expect to see `get_private_name` going.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1786
> +                fixEdge<SymbolUse>(node->child2());

I think we should blindly speculate symbol here when it is `GetPrivateName`, like we are doing for PutPrivateName. I don't think we are expecting to see any other type here.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1789
> +                fixEdge<CellUse>(node->child1());

Is there a reason we want to have UntypedUse for `base`? I don't think we have profitable paths there. We are always fixing base to cell for both `PutPrivateName` and `GetPrivateName`. This also would simplify a lot SpeculativeJIT code.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
> +    JSValueOperand base(this, m_graph.child(node, 0), ManualOperandSpeculation);

We are missing a Cell speculation here if `node->child1().useKind() == CellUse`. But if we decide to always speculate Cell for base (as proposed on Fixup) we cna simply use `SpeculateCellOperand`. Doing so helps on register pressure for 32-bits architectures.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3505
> +        resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(codeOrigin)), gen.stubInfo(), baseRegs, propertyRegs);

I'm not 100% sure if this is 32-bits friendly, since we only have payloadOnly for propertyRegs. We need to include CCallHelpers::CellValue there.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3531
> +    case UntypedUse: {

If we decide to always speculate cell on base, we don't need this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3947
> +        LValue property = lowJSValue(node->child2(), ManualOperandSpeculation);

We can make base always Cell and property always symbol here.

[Caio Lima]

(In reply to Caio Lima from comment #19)
> Comment on attachment 410149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410149&action=review
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1789
> > +                fixEdge<CellUse>(node->child1());
> 
> Is there a reason we want to have UntypedUse for `base`? I don't think we
> have profitable paths there. We are always fixing base to cell for both
> `PutPrivateName` and `GetPrivateName`. This also would simplify a lot
> SpeculativeJIT code.

I meant to say `PutPrivateName` and `PutPrivateNameById`.

[Caitlin Potter (:caitp)]

Comment on attachment 410149 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
>> +    JSValueOperand base(this, m_graph.child(node, 0), ManualOperandSpeculation);
> 
> We are missing a Cell speculation here if `node->child1().useKind() == CellUse`. But if we decide to always speculate Cell for base (as proposed on Fixup) we cna simply use `SpeculateCellOperand`. Doing so helps on register pressure for 32-bits architectures.

in the PutPrivateName case, you are just calling into operationPutPrivateNameGeneric.

Here, we need the tag register on 32bits for JITGetByValGenerator

[Caio Lima]

(In reply to Caitlin Potter (:caitp) from comment #21)
> Comment on attachment 410149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410149&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
> >> +    JSValueOperand base(this, m_graph.child(node, 0), ManualOperandSpeculation);
> > 
> > We are missing a Cell speculation here if `node->child1().useKind() == CellUse`. But if we decide to always speculate Cell for base (as proposed on Fixup) we cna simply use `SpeculateCellOperand`. Doing so helps on register pressure for 32-bits architectures.
> 
> in the PutPrivateName case, you are just calling into
> operationPutPrivateNameGeneric.
> 
> Here, we need the tag register on 32bits for JITGetByValGenerator

I'm not 100% sure about JITGetByValGenerator, but for JITGetByIdGenerator we don't need cell tag in some cases (see `SpeculativeJIT::compileGetById` when we speculate `CellUse` on base). Since they share a lot of code, I think we might not need base's tag for anything, since we always check/speculate before taking IC fast path, even on BaselineJIT. However, that's a different story for `property`.

[Caio Lima]

(In reply to Caio Lima from comment #22)
> (In reply to Caitlin Potter (:caitp) from comment #21)
> > Comment on attachment 410149 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=410149&action=review
> > 
> > >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
> > >> +    JSValueOperand base(this, m_graph.child(node, 0), ManualOperandSpeculation);
> > > 
> > > We are missing a Cell speculation here if `node->child1().useKind() == CellUse`. But if we decide to always speculate Cell for base (as proposed on Fixup) we cna simply use `SpeculateCellOperand`. Doing so helps on register pressure for 32-bits architectures.
> > 
> > in the PutPrivateName case, you are just calling into
> > operationPutPrivateNameGeneric.
> > 
> > Here, we need the tag register on 32bits for JITGetByValGenerator
> 
> I'm not 100% sure about JITGetByValGenerator, but for JITGetByIdGenerator we
> don't need cell tag in some cases (see `SpeculativeJIT::compileGetById` when
> we speculate `CellUse` on base). Since they share a lot of code, I think we
> might not need base's tag for anything, since we always check/speculate
> before taking IC fast path, even on BaselineJIT. However, that's a different
> story for `property`.

Just checked that we do `JITGetByValGenerator gen(
            m_codeBlock, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))), RegisterSet::stubUnavailableRegisters(),
            JSValueRegs::payloadOnly(regT0), JSValueRegs(regT3, regT2), JSValueRegs(regT1, regT0));` on `JIT::emit_op_get_by_val`. So it's fine to not having tag there on `get_private_name` as well.

[Saam Barati]

Comment on attachment 410149 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1789
>>> +                fixEdge<CellUse>(node->child1());
>> 
>> Is there a reason we want to have UntypedUse for `base`? I don't think we have profitable paths there. We are always fixing base to cell for both `PutPrivateName` and `GetPrivateName`. This also would simplify a lot SpeculativeJIT code.
> 
> I meant to say `PutPrivateName` and `PutPrivateNameById`.

the benefit of not always speculating is that throwing an exception is better than an OSR exit loop

[Caitlin Potter (:caitp)]

Comment on attachment 410149 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1789
>>>> +                fixEdge<CellUse>(node->child1());
>>> 
>>> Is there a reason we want to have UntypedUse for `base`? I don't think we have profitable paths there. We are always fixing base to cell for both `PutPrivateName` and `GetPrivateName`. This also would simplify a lot SpeculativeJIT code.
>> 
>> I meant to say `PutPrivateName` and `PutPrivateNameById`.
> 
> the benefit of not always speculating is that throwing an exception is better than an OSR exit loop

would it be possible to speculate optimistically until an error case is actually reached, and then generate the slightly more pessimistic code?

[Caitlin Potter (:caitp)]

Created attachment 410568 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 410782 [details]
Patch

Attempt to make 32bit bots happy

[Caitlin Potter (:caitp)]

Created attachment 410859 [details]
Patch

send JSCellTag as immediate to JITGetByVal stub on 32bits (from what I can see analyzing the DFG disassembly, it looks fine. .)

[Caitlin Potter (:caitp)]

Comment on attachment 410149 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/GetByStatus.cpp:111
>> +    case op_get_private_name:
> 
> I think this is something we'd like to add in future patches. DO you mind add a comment or even a FIXME to use IC information from LLInt to populate GetByStatus?
> 
> Also, can we double check that GetByIdStatus is doing the right thing for `get_private_name`? We have stronger requirements than common `get_by_[val|id]` because we need to throw exception when private field is not present in an object and never lookup prototype chain. Since we do Unset and Prototype load IC, it would be good to include ASSERTs on paths we don't expect to see `get_private_name` going.

I've added a comment and filed a bug --- however I'm not sure LLInt gives us much to work with currently, but it would be simple enough to add

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1786
>> +                fixEdge<SymbolUse>(node->child2());
> 
> I think we should blindly speculate symbol here when it is `GetPrivateName`, like we are doing for PutPrivateName. I don't think we are expecting to see any other type here.

we are now speculating symbol 100% of the time

>>>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1789
>>>>> +                fixEdge<CellUse>(node->child1());
>>>> 
>>>> Is there a reason we want to have UntypedUse for `base`? I don't think we have profitable paths there. We are always fixing base to cell for both `PutPrivateName` and `GetPrivateName`. This also would simplify a lot SpeculativeJIT code.
>>> 
>>> I meant to say `PutPrivateName` and `PutPrivateNameById`.
>> 
>> the benefit of not always speculating is that throwing an exception is better than an OSR exit loop
> 
> would it be possible to speculate optimistically until an error case is actually reached, and then generate the slightly more pessimistic code?

from the offline discussion, have decided not to be optimistic here, and emit a slow case for non-cells

>>>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
>>>>> +    JSValueOperand base(this, m_graph.child(node, 0), ManualOperandSpeculation);
>>>> 
>>>> We are missing a Cell speculation here if `node->child1().useKind() == CellUse`. But if we decide to always speculate Cell for base (as proposed on Fixup) we cna simply use `SpeculateCellOperand`. Doing so helps on register pressure for 32-bits architectures.
>>> 
>>> in the PutPrivateName case, you are just calling into operationPutPrivateNameGeneric.
>>> 
>>> Here, we need the tag register on 32bits for JITGetByValGenerator
>> 
>> I'm not 100% sure about JITGetByValGenerator, but for JITGetByIdGenerator we don't need cell tag in some cases (see `SpeculativeJIT::compileGetById` when we speculate `CellUse` on base). Since they share a lot of code, I think we might not need base's tag for anything, since we always check/speculate before taking IC fast path, even on BaselineJIT. However, that's a different story for `property`.
> 
> Just checked that we do `JITGetByValGenerator gen(
>             m_codeBlock, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))), RegisterSet::stubUnavailableRegisters(),
>             JSValueRegs::payloadOnly(regT0), JSValueRegs(regT3, regT2), JSValueRegs(regT1, regT0));` on `JIT::emit_op_get_by_val`. So it's fine to not having tag there on `get_private_name` as well.

per the long discussion offline, it's going to be messy to avoid allocating a tag register here, and will require some fixes to JSValueOperand. I prefer to leave this alone for now, to keep the patch as minimal as possible. We can revisit this later.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3498
> +        baseRegs, propertyRegs, resultRegs);

specifying payloadOnly for baseRegs is possible, but it won't be helpful due to the issue mentioned above as discussed offline. Tabling that for now. I have updated propertyRegs to use only the payload, and verified that this is doing the right thing in the DFG disassembly on 32 bits.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3531
>> +    case UntypedUse: {
> 
> If we decide to always speculate cell on base, we don't need this.

as discussed offline with Saam, we're going to prefer emitting a branch+slow case when Cell is not guaranteed, rather than OSRing.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3947
>> +        LValue property = lowJSValue(node->child2(), ManualOperandSpeculation);
> 
> We can make base always Cell and property always symbol here.

My understanding of this part of the pipeline is even more limited than my understanding of DFG, so I'm not 100% sure I've done the work here adequately, but please take a look at the changes that were made.

[Saam Barati]

Comment on attachment 410859 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4756
> +        // 1) Emit prototype structure checks for all chains. This could sort of maybe not be
> +        //    optimal, if there is some rarely executed case in the chain that requires a lot
> +        //    of checks and those checks are not watchpointable.

This comment doesn't seem to match what the code is doing.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4757
> +        for (const GetByIdVariant& variant : getByStatus.variants()) {

Do we want to assert the variant is a self load?
(Similar assert in Constant Folding too?)

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4795
> +    if (!variant.callLinkStatus() && variant.intrinsic() == NoIntrinsic)

should we assert there is no callLinkSatus? Why would a private name access ever have a call?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6460
> +                    m_graph.m_slowGetByVal.add(node);

You add to this hashset, but never read from it in DFGSpeculatiiveJIT/FTLLower

We should either not do this if we don't think it's profitable, or we should read from the hash set in DFGSpeculativeJIT/FTLLower to emit different code

> Source/JavaScriptCore/runtime/OptionsList.h:574
> +    v(usePrivateClassFields, SupportsFTL | SupportsDFG, "https://bugs.webkit.org/show_bug.cgi?id=212781", "https://bugs.webkit.org/show_bug.cgi?id=212784")

Do we support Put?

[Caitlin Potter (:caitp)]

Comment on attachment 410859 [details]
Patch

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

I've taken care of these comments while rebasing today

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4757
>> +        for (const GetByIdVariant& variant : getByStatus.variants()) {
> 
> Do we want to assert the variant is a self load?
> (Similar assert in Constant Folding too?)

is that not what is happening on line 4759 below already?

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4795
>> +    if (!variant.callLinkStatus() && variant.intrinsic() == NoIntrinsic)
> 
> should we assert there is no callLinkSatus? Why would a private name access ever have a call?

I have no idea here --- I think I left this open for private accessors, but since I'm not positive if it's actually needed for that feature or not, it may be fine to remove it.

I've done that for now, and I guess we can re-add it if it does end up being needed.

AFAIK there's no chance of this being an intrinsic either, at the moment, either, but it's such a minor thing I don't think it matters much

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6460
>> +                    m_graph.m_slowGetByVal.add(node);
> 
> You add to this hashset, but never read from it in DFGSpeculatiiveJIT/FTLLower
> 
> We should either not do this if we don't think it's profitable, or we should read from the hash set in DFGSpeculativeJIT/FTLLower to emit different code

looking at it, I don't expect this to be worth using (I think the sorts of cases making use of it for GetByVal are mostly things which will never be optimized here)

[Caitlin Potter (:caitp)]

Comment on attachment 410859 [details]
Patch

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

I've taken care of these comments while rebasing today

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4757
>> +        for (const GetByIdVariant& variant : getByStatus.variants()) {
> 
> Do we want to assert the variant is a self load?
> (Similar assert in Constant Folding too?)

is that not what is happening on line 4759 below already?

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4795
>> +    if (!variant.callLinkStatus() && variant.intrinsic() == NoIntrinsic)
> 
> should we assert there is no callLinkSatus? Why would a private name access ever have a call?

I have no idea here --- I think I left this open for private accessors, but since I'm not positive if it's actually needed for that feature or not, it may be fine to remove it.

I've done that for now, and I guess we can re-add it if it does end up being needed.

AFAIK there's no chance of this being an intrinsic either, at the moment, either, but it's such a minor thing I don't think it matters much

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6460
>> +                    m_graph.m_slowGetByVal.add(node);
> 
> You add to this hashset, but never read from it in DFGSpeculatiiveJIT/FTLLower
> 
> We should either not do this if we don't think it's profitable, or we should read from the hash set in DFGSpeculativeJIT/FTLLower to emit different code

looking at it, I don't expect this to be worth using (I think the sorts of cases making use of it for GetByVal are mostly things which will never be optimized here)

[Caitlin Potter (:caitp)]

Created attachment 411457 [details]
Patch

I've addressed these comments as mentioned in the comments, if that's everything I think this should be okay

[Filip Pizlo]

Comment on attachment 410859 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4728
> +void ByteCodeParser::handleGetPrivateNameById(

Why isn’t this a mode of handleGetById?  It has some of the same logic, like how it parses GetByIdStatus. Could just make handleGetById know about what this function does as just a mode, triggered by an enum arg for example.

[Filip Pizlo]

Comment on attachment 411457 [details]
Patch

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

This looks pretty reasonable. I think the only thing I’d change is to use handleGetById instead of adding a new function. Just give that thing an enum arg or whatever to tell it to do private mode, and gate a bunch of stuff in there on that. That keeps you from duplicating logic that I think is a bit gnarly. R=me with that comment.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3477
> +    speculate(node, node->child1());

This is weird. Can’t imagine this doing smart codegen. Why aren’t we doing SpeculateCellOperand?

[Filip Pizlo]

(In reply to Filip Pizlo from comment #35)
> Comment on attachment 411457 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411457&action=review
> 
> This looks pretty reasonable. I think the only thing I’d change is to use
> handleGetById instead of adding a new function. Just give that thing an enum
> arg or whatever to tell it to do private mode, and gate a bunch of stuff in
> there on that. That keeps you from duplicating logic that I think is a bit
> gnarly. R=me with that comment.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3477
> > +    speculate(node, node->child1());
> 
> This is weird. Can’t imagine this doing smart codegen. Why aren’t we doing
> SpeculateCellOperand?

I mean, R=me with two comments. Gotta do something better than speculate() there.

[Saam Barati]

Comment on attachment 411457 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3476
> +    JSValueOperand base(this, m_graph.child(node, 0), ManualOperandSpeculation);

nit: A bit weird to use graph to access the child here, but differently below

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3477
>>> +    speculate(node, node->child1());
>> 
>> This is weird. Can’t imagine this doing smart codegen. Why aren’t we doing SpeculateCellOperand?
> 
> I mean, R=me with two comments. Gotta do something better than speculate() there.

I think this codegen should actually be sensible. It should just use the register if it's already allocated.

[Saam Barati]

Comment on attachment 410859 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4795
>>>> +    if (!variant.callLinkStatus() && variant.intrinsic() == NoIntrinsic)
>>> 
>>> should we assert there is no callLinkSatus? Why would a private name access ever have a call?
>> 
>> I have no idea here --- I think I left this open for private accessors, but since I'm not positive if it's actually needed for that feature or not, it may be fine to remove it.
>> 
>> I've done that for now, and I guess we can re-add it if it does end up being needed.
>> 
>> AFAIK there's no chance of this being an intrinsic either, at the moment, either, but it's such a minor thing I don't think it matters much
> 
> I have no idea here --- I think I left this open for private accessors, but since I'm not positive if it's actually needed for that feature or not, it may be fine to remove it.
> 
> I've done that for now, and I guess we can re-add it if it does end up being needed.
> 
> AFAIK there's no chance of this being an intrinsic either, at the moment, either, but it's such a minor thing I don't think it matters much

Right, but there isn't the any code that'd actually handle the getter if one existed. I agree it'd probably be used in the future. Sounds like Phil's suggestion to integrate into GetById handling code might just work and be a bit future proofed for private accessors as well

>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6460
>>>> +                    m_graph.m_slowGetByVal.add(node);
>>> 
>>> You add to this hashset, but never read from it in DFGSpeculatiiveJIT/FTLLower
>>> 
>>> We should either not do this if we don't think it's profitable, or we should read from the hash set in DFGSpeculativeJIT/FTLLower to emit different code
>> 
>> looking at it, I don't expect this to be worth using (I think the sorts of cases making use of it for GetByVal are mostly things which will never be optimized here)
> 
> looking at it, I don't expect this to be worth using (I think the sorts of cases making use of it for GetByVal are mostly things which will never be optimized here)

I agree. Let's skip adding to the hashset

[Caitlin Potter (:caitp)]

Comment on attachment 411457 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3477
>>>> +    speculate(node, node->child1());
>>> 
>>> This is weird. Can’t imagine this doing smart codegen. Why aren’t we doing SpeculateCellOperand?
>> 
>> I mean, R=me with two comments. Gotta do something better than speculate() there.
> 
> I think this codegen should actually be sensible. It should just use the register if it's already allocated.

I generally agree that this isn't quite right, but I also don't think it's likely to do anything super problematic.

I'm weighing a few different options, and I'd appreciate some feedback on what is preferred

1) generate the CellUse and UntypedUse cases separately (as is done in GetPrivateNameById), which would let us get away with using SpeculateCellOperand for CellUses, but would be bigger/messier.

2) leave it as-is (just maybe make some changes to get edges more consistently everywhere in the function), a mix of what the two of you are saying. The register allocation is probably not perfect on 32bit (in the cases I've looked at, it does allocate a tag, although I can't guarantee it will in all other cases). There is potentially a chance to produce speculation checks for unwanted types, but I'm not 100% sure that this can actually happen (if it can, it's the kind of thing that would lead to the DFG_ASSERT in compileGetPrivateNameById) --- if it is possible, it could be fixed in the DFGFixupPhase easy enough.

3) get rid of the CellUse specialization entirely --- just assume UntypedUse. We get a tag register, whatever. This is simple, and produces similar register allocation on 32bit, and removes any chance of an OSRExit (which would make some test names weird)

So there are sort of tradeoffs for all of that. My personal preference, since we don't have real world usage data to justify putting effort in, would be to keep it simple --- I think we probably still do just as well on the benchmarks we have only handling the UntypedUse case, and we can keep the bit which tries to eliminate a redundant type check if we know the incoming value is a Cell.

[Caitlin Potter (:caitp)]

Comment on attachment 411457 [details]
Patch

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

>>>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3477
>>>>> +    speculate(node, node->child1());
>>>> 
>>>> This is weird. Can’t imagine this doing smart codegen. Why aren’t we doing SpeculateCellOperand?
>>> 
>>> I mean, R=me with two comments. Gotta do something better than speculate() there.
>> 
>> I think this codegen should actually be sensible. It should just use the register if it's already allocated.
> 
> I generally agree that this isn't quite right, but I also don't think it's likely to do anything super problematic.
> 
> I'm weighing a few different options, and I'd appreciate some feedback on what is preferred
> 
> 1) generate the CellUse and UntypedUse cases separately (as is done in GetPrivateNameById), which would let us get away with using SpeculateCellOperand for CellUses, but would be bigger/messier.
> 
> 2) leave it as-is (just maybe make some changes to get edges more consistently everywhere in the function), a mix of what the two of you are saying. The register allocation is probably not perfect on 32bit (in the cases I've looked at, it does allocate a tag, although I can't guarantee it will in all other cases). There is potentially a chance to produce speculation checks for unwanted types, but I'm not 100% sure that this can actually happen (if it can, it's the kind of thing that would lead to the DFG_ASSERT in compileGetPrivateNameById) --- if it is possible, it could be fixed in the DFGFixupPhase easy enough.
> 
> 3) get rid of the CellUse specialization entirely --- just assume UntypedUse. We get a tag register, whatever. This is simple, and produces similar register allocation on 32bit, and removes any chance of an OSRExit (which would make some test names weird)
> 
> So there are sort of tradeoffs for all of that. My personal preference, since we don't have real world usage data to justify putting effort in, would be to keep it simple --- I think we probably still do just as well on the benchmarks we have only handling the UntypedUse case, and we can keep the bit which tries to eliminate a redundant type check if we know the incoming value is a Cell.

By "keep it simple", I'm referring to option 3, and only handle the UntypedUse case here.

[Caitlin Potter (:caitp)]

Created attachment 411901 [details]
Patch

This accesses the child edges in a consistent way everywhere --- and adds a little bit of complexity by using a SpeculateCellOperand when we have a CellUse. Cleans some other things up a bit, adds some asserts, and is verified to not allocate a long-lasting tag operand on 32bit when speculate Cell for base

[Caitlin Potter (:caitp)]

Comment on attachment 411457 [details]
Patch

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

>>>>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3477
>>>>>> +    speculate(node, node->child1());
>>>>> 
>>>>> This is weird. Can’t imagine this doing smart codegen. Why aren’t we doing SpeculateCellOperand?
>>>> 
>>>> I mean, R=me with two comments. Gotta do something better than speculate() there.
>>> 
>>> I think this codegen should actually be sensible. It should just use the register if it's already allocated.
>> 
>> I generally agree that this isn't quite right, but I also don't think it's likely to do anything super problematic.
>> 
>> I'm weighing a few different options, and I'd appreciate some feedback on what is preferred
>> 
>> 1) generate the CellUse and UntypedUse cases separately (as is done in GetPrivateNameById), which would let us get away with using SpeculateCellOperand for CellUses, but would be bigger/messier.
>> 
>> 2) leave it as-is (just maybe make some changes to get edges more consistently everywhere in the function), a mix of what the two of you are saying. The register allocation is probably not perfect on 32bit (in the cases I've looked at, it does allocate a tag, although I can't guarantee it will in all other cases). There is potentially a chance to produce speculation checks for unwanted types, but I'm not 100% sure that this can actually happen (if it can, it's the kind of thing that would lead to the DFG_ASSERT in compileGetPrivateNameById) --- if it is possible, it could be fixed in the DFGFixupPhase easy enough.
>> 
>> 3) get rid of the CellUse specialization entirely --- just assume UntypedUse. We get a tag register, whatever. This is simple, and produces similar register allocation on 32bit, and removes any chance of an OSRExit (which would make some test names weird)
>> 
>> So there are sort of tradeoffs for all of that. My personal preference, since we don't have real world usage data to justify putting effort in, would be to keep it simple --- I think we probably still do just as well on the benchmarks we have only handling the UntypedUse case, and we can keep the bit which tries to eliminate a redundant type check if we know the incoming value is a Cell.
> 
> By "keep it simple", I'm referring to option 3, and only handle the UntypedUse case here.

The changes/interdiff from the latest patch (for landing) are in https://gist.github.com/caitp/96f175a31b7da1b763438b741933fe96

[Caitlin Potter (:caitp)]

Created attachment 411977 [details]
Patch for landing

[EWS]

Committed r268794: <https://trac.webkit.org/changeset/268794>

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

[Saam Barati]

Comment on attachment 411977 [details]
Patch for landing

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

> Source/JavaScriptCore/ChangeLog:8
> +        Adds DFG/FTL support for op_get_private_name.

What’s the speed up on your microbenchmarks?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3508
> +    const bool baseIsKnownCell = m_state.forNode(m_graph.child(node, 0)).isType(SpecCell);

The better way to do this is to check the use kind of the base edge

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3539
> +    switch (m_graph.child(node, 0).useKind()) {

Nit: you use m_graph.node in various places here. I’d use the child1/child2 accessor on node itself, since you know the node type and you know that it’s not varargs

> Source/JavaScriptCore/jit/JITOperations.cpp:2378
> +        return JSValue::encode(slot.getValue(globalObject, fieldName));

I think you want to release the exception scope before calling getValhe here

[Caitlin Potter (:caitp)]

Comment on attachment 411977 [details]
Patch for landing

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Adds DFG/FTL support for op_get_private_name.
> 
> What’s the speed up on your microbenchmarks?

I can see how it's helpful to include exact figures in the changelog, but I knew the figures were at least 1.5x to 2x faster based on earlier runs, and have improved since then.

Here's a run on a fairly busy i9-9900K, I'll do some runs on an ARM devices as well shortly.

```
                                       GPN_NoDFG                 GPN_YesDFG

get-private-name                   689.4523+-7.9868     ^    117.3869+-3.7813        ^ definitely 5.8733x faster
monomorphic-get-private-field       69.1436+-1.3581     ^     19.5125+-2.4176        ^ definitely 3.5436x faster
polymorphic-get-private-field      189.1932+-2.8165     ^     50.2354+-2.1225        ^ definitely 3.7661x faster
polymorphic-put-private-field      138.4540+-3.1889     ^     34.8745+-1.8817        ^ definitely 3.9701x faster
put-private-field                   49.8130+-6.8268     ^     20.3215+-1.2123        ^ definitely 2.4512x faster

<geometric>                        144.0651+-4.4416     ^     38.1988+-1.3225        ^ definitely 3.7715x faster
```

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3508
>> +    const bool baseIsKnownCell = m_state.forNode(m_graph.child(node, 0)).isType(SpecCell);
> 
> The better way to do this is to check the use kind of the base edge

Does it really make a huge difference? I assume the abstract value is also informed by DFGFixupPhase, isn't it? I think the way you're suggesting is more obvious and I'm happy to change it, but I've used this one for consistency with other code, which similarly relies on it instead of the use kind for whatever reason.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3539
>> +    switch (m_graph.child(node, 0).useKind()) {
> 
> Nit: you use m_graph.node in various places here. I’d use the child1/child2 accessor on node itself, since you know the node type and you know that it’s not varargs

My thinking is it doesn't really matter, and one is a bit looser than the other in terms of requirements, and is also the one which seems to be preferred in this file. 🤷‍♀️

>> Source/JavaScriptCore/jit/JITOperations.cpp:2378
>> +        return JSValue::encode(slot.getValue(globalObject, fieldName));
> 
> I think you want to release the exception scope before calling getValhe here

I agree, I'll fix up in a followup

[Caitlin Potter (:caitp)]

Comment on attachment 411977 [details]
Patch for landing

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

>>> Source/JavaScriptCore/ChangeLog:8
>>> +        Adds DFG/FTL support for op_get_private_name.
>> 
>> What’s the speed up on your microbenchmarks?
> 
> I can see how it's helpful to include exact figures in the changelog, but I knew the figures were at least 1.5x to 2x faster based on earlier runs, and have improved since then.
> 
> Here's a run on a fairly busy i9-9900K, I'll do some runs on an ARM devices as well shortly.
> 
> ```
>                                        GPN_NoDFG                 GPN_YesDFG
> 
> get-private-name                   689.4523+-7.9868     ^    117.3869+-3.7813        ^ definitely 5.8733x faster
> monomorphic-get-private-field       69.1436+-1.3581     ^     19.5125+-2.4176        ^ definitely 3.5436x faster
> polymorphic-get-private-field      189.1932+-2.8165     ^     50.2354+-2.1225        ^ definitely 3.7661x faster
> polymorphic-put-private-field      138.4540+-3.1889     ^     34.8745+-1.8817        ^ definitely 3.9701x faster
> put-private-field                   49.8130+-6.8268     ^     20.3215+-1.2123        ^ definitely 2.4512x faster
> 
> <geometric>                        144.0651+-4.4416     ^     38.1988+-1.3225        ^ definitely 3.7715x faster
> ```

ARMv7 build:
ELF 32-bit LSB pie executable, ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0, BuildID[sha1]=27ac4fa3b95eabeedff9423161aec226fa8ee5ed, not stripped
Linux bbox-10-armhf 5.6.0-0.bpo.2-arm64 #1 SMP Debian 5.6.14-2~bpo10+1 (2020-06-09) armv8l GNU/Linux (X-gene 2 w/ max 3300hz)

```
                                       GPN_NoDFG                 GPN_YesDFG

polymorphic-get-private-field     2342.5698+-67.0495    ^   1174.6169+-1.1972        ^ definitely 1.9943x faster
get-private-name                  4085.7298+-126.2157   ^    437.1069+-0.8109        ^ definitely 9.3472x faster
monomorphic-get-private-field      449.7250+-23.7829    ^     64.5152+-0.7488        ^ definitely 6.9708x faster
polymorphic-put-private-field     1397.2725+-55.7635    ^    653.0471+-33.7018       ^ definitely 2.1396x faster
put-private-field                  290.6740+-14.2945    ^     59.0010+-0.6738        ^ definitely 4.9266x faster

<geometric>                       1118.0396+-30.2615    ^    263.7305+-2.8495        ^ definitely 4.2393x faster
```

[Saam Barati]

Comment on attachment 411977 [details]
Patch for landing

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

>>>> Source/JavaScriptCore/ChangeLog:8
>>>> +        Adds DFG/FTL support for op_get_private_name.
>>> 
>>> What’s the speed up on your microbenchmarks?
>> 
>> I can see how it's helpful to include exact figures in the changelog, but I knew the figures were at least 1.5x to 2x faster based on earlier runs, and have improved since then.
>> 
>> Here's a run on a fairly busy i9-9900K, I'll do some runs on an ARM devices as well shortly.
>> 
>> ```
>>                                        GPN_NoDFG                 GPN_YesDFG
>> 
>> get-private-name                   689.4523+-7.9868     ^    117.3869+-3.7813        ^ definitely 5.8733x faster
>> monomorphic-get-private-field       69.1436+-1.3581     ^     19.5125+-2.4176        ^ definitely 3.5436x faster
>> polymorphic-get-private-field      189.1932+-2.8165     ^     50.2354+-2.1225        ^ definitely 3.7661x faster
>> polymorphic-put-private-field      138.4540+-3.1889     ^     34.8745+-1.8817        ^ definitely 3.9701x faster
>> put-private-field                   49.8130+-6.8268     ^     20.3215+-1.2123        ^ definitely 2.4512x faster
>> 
>> <geometric>                        144.0651+-4.4416     ^     38.1988+-1.3225        ^ definitely 3.7715x faster
>> ```
> 
> ARMv7 build:
> ELF 32-bit LSB pie executable, ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0, BuildID[sha1]=27ac4fa3b95eabeedff9423161aec226fa8ee5ed, not stripped
> Linux bbox-10-armhf 5.6.0-0.bpo.2-arm64 #1 SMP Debian 5.6.14-2~bpo10+1 (2020-06-09) armv8l GNU/Linux (X-gene 2 w/ max 3300hz)
> 
> ```
>                                        GPN_NoDFG                 GPN_YesDFG
> 
> polymorphic-get-private-field     2342.5698+-67.0495    ^   1174.6169+-1.1972        ^ definitely 1.9943x faster
> get-private-name                  4085.7298+-126.2157   ^    437.1069+-0.8109        ^ definitely 9.3472x faster
> monomorphic-get-private-field      449.7250+-23.7829    ^     64.5152+-0.7488        ^ definitely 6.9708x faster
> polymorphic-put-private-field     1397.2725+-55.7635    ^    653.0471+-33.7018       ^ definitely 2.1396x faster
> put-private-field                  290.6740+-14.2945    ^     59.0010+-0.6738        ^ definitely 4.9266x faster
> 
> <geometric>                       1118.0396+-30.2615    ^    263.7305+-2.8495        ^ definitely 4.2393x faster
> ```

Nice!!

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3508
>>> +    const bool baseIsKnownCell = m_state.forNode(m_graph.child(node, 0)).isType(SpecCell);
>> 
>> The better way to do this is to check the use kind of the base edge
> 
> Does it really make a huge difference? I assume the abstract value is also informed by DFGFixupPhase, isn't it? I think the way you're suggesting is more obvious and I'm happy to change it, but I've used this one for consistency with other code, which similarly relies on it instead of the use kind for whatever reason.

Not a huge difference, but it does make a difference. That other code could be made better too. My reasoning here is as such (and could actually result in bugs on 32-bit):
- AI is conservative. It might not always have the type proof you want it to. If you have external info that's guaranteed to be at least as specific as AI, then that info is better to use
- Since you only provide the payload for 32-bit code with CellUse, you'd be SOL if you tried to do the branch below on a CellUse edge. Better to check the same thing in both places, then two different things in both places.
- This should really be checking SpecCellCheck, not SpecCell, since that's what branchIfCell/branchIfNotCell is checking, and is also what CellUse guarantees you

[Caio Lima]

*** Bug 212781 has been marked as a duplicate of this bug. ***