Bug 122617 – transition void cti_op_* methods to JIT operations.

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

[Michael Saboff]

Transition these stubs to operations
    cti_handle_watchdog_timer
    cti_op_debug
    cti_op_pop_scope
    cti_op_profile_did_call
    cti_op_profile_will_call
    cti_op_put_by_index
    cti_op_put_getter_setter
    cti_op_tear_off_activation
    cti_op_tear_off_arguments
    cti_op_throw_static_error
    cti_optimize

[Michael Saboff]

I'll also transition cgi_op_push_name_scope and cti_push_with_scope.

[Michael Saboff]

Created attachment 214213 [details]
Patch

[Mark Lam]

Comment on attachment 214213 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOpcodes.cpp:262
> -    stubCall.addArgument(unmodifiedArgumentsRegister(VirtualRegister(arguments)).offset(), regT2);
> -    stubCall.addArgument(activation, regT2);
> -    stubCall.call();
> +    emitGetVirtualRegister(arguments, regT0);

The code for unmodifiedArgumentsRegister says:
    inline VirtualRegister unmodifiedArgumentsRegister(VirtualRegister argumentsRegister) { return VirtualRegister(argumentsRegister.offset() + 1); }

Is it appropriate to replace "stubCall.addArgument(unmodifiedArgumentsRegister(VirtualRegister(arguments)).offset(), regT2);" with "emitGetVirtualRegister(arguments, regT0);"?

> Source/JavaScriptCore/jit/JITOperations.h:239
> -size_t JIT_OPERATION operationCompareLess(ExecState*, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) WTF_INTERNAL;
> -size_t JIT_OPERATION operationCompareLessEq(ExecState*, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) WTF_INTERNAL;
> -size_t JIT_OPERATION operationCompareGreater(ExecState*, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) WTF_INTERNAL;
> -size_t JIT_OPERATION operationCompareGreaterEq(ExecState*, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) WTF_INTERNAL;
> +size_t JIT_OPERATION operationCompareLess(ExecState*, EncodedJSValue, EncodedJSValue encodedOp2) WTF_INTERNAL;
> +size_t JIT_OPERATION operationCompareLessEq(ExecState*, EncodedJSValue, EncodedJSValue encodedOp2) WTF_INTERNAL;
> +size_t JIT_OPERATION operationCompareGreater(ExecState*, EncodedJSValue, EncodedJSValue encodedOp2) WTF_INTERNAL;
> +size_t JIT_OPERATION operationCompareGreaterEq(ExecState*, EncodedJSValue, EncodedJSValue encodedOp2) WTF_INTERNAL;

Did you also intend to remove the encodedOp2 as well?

[Michael Saboff]

Created attachment 214265 [details]
With fixes Mark suggested

[Geoffrey Garen]

Comment on attachment 214265 [details]
With fixes Mark suggested

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

> Source/JavaScriptCore/jit/CCallHelpers.h:1096
> +        poke(arg4, POKE_ARGUMENT_OFFSET);

Will this poke be valid when we're running on the C stack? Will each stack frame reserve space for these pokes?

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1185
> +    callOperation(operationProfileWillCall, regT1, regT0);

Wrong function = broken profiler. Did the profiler tests pass for this patch?

[Michael Saboff]

Created attachment 214280 [details]
Updated patch.  Rebased and with fix.

(In reply to comment #5)
> (From update of attachment 214265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214265&action=review
> 
> > Source/JavaScriptCore/jit/CCallHelpers.h:1096
> > +        poke(arg4, POKE_ARGUMENT_OFFSET);
> 
> Will this poke be valid when we're running on the C stack? Will each stack frame reserve space for these pokes?

This poke is valid way to pass additional arguments.  When we move JSC over to the C stack, we'll need to make sure there is space for calling out to C including any calls made by the immediate callee.  My thinking is that we make sure there is stack space for C calls beyond which JSC can use for stack frames.
 
> > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1185
> > +    callOperation(operationProfileWillCall, regT1, regT0);
> 
> Wrong function = broken profiler. Did the profiler tests pass for this patch?

I made the change and ran the layout tests including the profiler tests with a 32 bit build.

[Geoffrey Garen]

Comment on attachment 214280 [details]
Updated patch.  Rebased and with fix.

r=me

[Michael Saboff]

Committed r157457: <http://trac.webkit.org/changeset/157457>