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.
Working on it now.
Created attachment 263458 [details] Patch WIP: DFG part is done
Created attachment 263461 [details] Patch WIP: DFG / FTL. Tests are not added yet.
Created attachment 263464 [details] Patch WIP: fixing 32bit
Created attachment 263474 [details] Patch WIP: fixing 32bit, part2
Created attachment 263475 [details] Patch WIP: fixing 32bit, part3
Created attachment 263478 [details] Patch
Created attachment 263495 [details] Patch
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.
Could anyone take a look? :)
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.
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.
Committed r191500: <http://trac.webkit.org/changeset/191500>
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
(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.
After inserting barriers, the failure is removed. I'll check what happens and barrier is necessary or not when calling the operations.
(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...
Ah, I misunderstood callOperation in DFG. I'll fix the patch and update it.
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.
Created attachment 264063 [details] Patch
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.
Comment on attachment 264063 [details] Patch r=me
Committed r191621: <http://trac.webkit.org/changeset/191621>
(In reply to comment #22) > Comment on attachment 264063 [details] > Patch > > r=me Thank you so much!!! :D