Bug 148860 – [ES6] Add DFG/FTL support for accessor put operations

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

[Yusuke Suzuki]

Currently, accessor put operations (like op_put_getter_by_id) are not compiled in DFG / FTL backend.
We should add them even if the implementation in DFG becomes just calling the runtime function,
since the code including these operations is not compiled in DFG even if the operation is not executed.

[Yusuke Suzuki]

Working on it now.

[Yusuke Suzuki]

Created attachment 263458 [details]
Patch

WIP: DFG part is done

[Yusuke Suzuki]

Created attachment 263461 [details]
Patch

WIP: DFG / FTL. Tests are not added yet.

[Yusuke Suzuki]

Created attachment 263464 [details]
Patch

WIP: fixing 32bit

[Yusuke Suzuki]

Created attachment 263474 [details]
Patch

WIP: fixing 32bit, part2

[Yusuke Suzuki]

Created attachment 263475 [details]
Patch

WIP: fixing 32bit, part3

[Yusuke Suzuki]

Created attachment 263478 [details]
Patch

[Yusuke Suzuki]

Created attachment 263495 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 263495 [details]
Patch

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

Added comments :)

> Source/JavaScriptCore/dfg/DFGStoreBarrierInsertionPhase.cpp:280
> +                considerBarrier(m_node->child1());

They store a new property to a base object.

> Source/JavaScriptCore/jit/JITOperations.h:-204
> -typedef void JIT_OPERATION (*V_JITOperation_EJIdZJJ)(ExecState*, EncodedJSValue, Identifier*, int32_t, EncodedJSValue, EncodedJSValue);

Change Identifier* to UniquedStringImpl* to use these operations in DFG / FTL.

> Source/JavaScriptCore/jit/JITOperations.h:315
> +void JIT_OPERATION operationPutSetterById(ExecState*, JSCell*, UniquedStringImpl*, int32_t options, JSCell*) WTF_INTERNAL;

These operations require some of arguments are JSObject.
Since the representation of EncodedJSValue containing JSCell* in JSVALUE64 environment is the same to JSCell*, we take JSCell* directly instead of EncodedJSValue.
It unifies operationPut{Getter,Setter}By{Id, Val} for JSVALUE64 and JSVALUE32_64.

[Yusuke Suzuki]

Could anyone take a look? :)

[Geoffrey Garen]

Comment on attachment 263495 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGNodeType.h:186
> +    macro(PutAccessorsById, NodeMustGenerate) \

I would call this "PutGetterSetterById" to match the opcode name.

If you want to rename the opcode in a separate patch, I'm OK with that. I would say "accessor" instead of "accessors" because there is only one accessor property descriptor, which holds both a getter and a setter.

>> Source/JavaScriptCore/dfg/DFGStoreBarrierInsertionPhase.cpp:280
>> +                considerBarrier(m_node->child1());
> 
> They store a new property to a base object.

I don't think we need a barrier in JIT code since we'll do the barrier in C++ code. We only need a barrier in JIT code if we emit JIT code to perform a write.

If we had a way to express it, we might want to tell this phase "I did a store barrier, so you can eliminate others". But I don't think we have that.

[Yusuke Suzuki]

Comment on attachment 263495 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/dfg/DFGNodeType.h:186
>> +    macro(PutAccessorsById, NodeMustGenerate) \
> 
> I would call this "PutGetterSetterById" to match the opcode name.
> 
> If you want to rename the opcode in a separate patch, I'm OK with that. I would say "accessor" instead of "accessors" because there is only one accessor property descriptor, which holds both a getter and a setter.

Thanks. Right.
Your pointing has changed my mind :). "PutGetterSetterById" seems better because we actually stores "GetterSetter" object as a property.
BTW, I think renaming current opcode "op_put_getter_setter" to "op_put_getter_setter_by_id" is nice. I'll upload this as a separate patch.

>>> Source/JavaScriptCore/dfg/DFGStoreBarrierInsertionPhase.cpp:280
>>> +                considerBarrier(m_node->child1());
>> 
>> They store a new property to a base object.
> 
> I don't think we need a barrier in JIT code since we'll do the barrier in C++ code. We only need a barrier in JIT code if we emit JIT code to perform a write.
> 
> If we had a way to express it, we might want to tell this phase "I did a store barrier, so you can eliminate others". But I don't think we have that.

Nice catch! You're right. These barriers are not necessary. Dropped.

[Yusuke Suzuki]

Committed r191500: <http://trac.webkit.org/changeset/191500>

[Ryan Haddad]

This change may have introduced 2 new JSC failures:
	stress/try-catch-stub-routine-replaced.js.dfg-eager-no-cjit-validate
	stress/try-catch-stub-routine-replaced.js.ftl-eager-no-cjit

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/354/steps/jscore-test/logs/stdio

[Yusuke Suzuki]

(In reply to comment #14)
> This change may have introduced 2 new JSC failures:
> 	stress/try-catch-stub-routine-replaced.js.dfg-eager-no-cjit-validate
> 	stress/try-catch-stub-routine-replaced.js.ftl-eager-no-cjit
> 
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/354/steps/jscore-
> test/logs/stdio

Thanks! I'm investigating it now.

[Yusuke Suzuki]

After inserting barriers, the failure is removed.
I'll check what happens and barrier is necessary or not when calling the operations.

[Yusuke Suzuki]

(In reply to comment #16)
> After inserting barriers, the failure is removed.
> I'll check what happens and barrier is necessary or not when calling the
> operations.

Barrier is not necessary, but I guess referencing this node may have any effect, still investigating...

[Yusuke Suzuki]

Ah, I misunderstood callOperation in DFG. I'll fix the patch and update it.

[Yusuke Suzuki]

OK, i've found the issue!
Before calling flushRegisters(), we should fill gprs by calling such as base.gpr() manually.

Wrong case:
flushRegister();
callOperation(..., base.gpr());  // No! base.gpr() may be filled right now! (After calling flushRegister())

Correct case:
GPRReg baseGPR = base.gpr();
flushRegister();
callOperation(..., baseGPR);  // OK!


In the wrong case, since we fill a register after calling flushRegister(), after calling callOperation(...), we may misdecide that a VirtualRegister is in a machine register in spite of callOperation's C call breaks caller save registers.

[Yusuke Suzuki]

Created attachment 264063 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 264063 [details]
Patch

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

OK, updated the patch. Could you take an another look?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6743
>  

Differences are these parts.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6752
> +    flushRegisters();

Before calling flushRegisters(), we should call such as `base.gpr()` to fill registers.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6770
> +    flushRegisters();

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6780
> +    JSValueRegs setterRegs = setter.jsValueRegs();

Note that JSValueRegs fills pair of registers correctly.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6782
> +    flushRegisters();

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6802
> +    flushRegisters();

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6809
> +    flushRegisters();

Ditto.

[Michael Saboff]

Comment on attachment 264063 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r191621: <http://trac.webkit.org/changeset/191621>

[Yusuke Suzuki]

(In reply to comment #22)
> Comment on attachment 264063 [details]
> Patch
> 
> r=me

Thank you so much!!! :D