Bug 122617

Summary:

transition void cti_op_* methods to JIT operations.

Product:

WebKit

Reporter:

Michael Saboff <msaboff>

Component:

JavaScriptCore

Assignee:

Michael Saboff <msaboff>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mark.lam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

122287

Attachments:

Description	Flags
Patch	none
With fixes Mark suggested	ggaren: review-
Updated patch. Rebased and with fix.	ggaren: review+

Michael Saboff

Reported 2013-10-10 13:09:22 PDT

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

Attachments
Patch (77.54 KB, patch) 2013-10-14 18:00 PDT, Michael Saboff	no flags	Details Formatted Diff Diff
With fixes Mark suggested (77.55 KB, patch) 2013-10-15 08:53 PDT, Michael Saboff	ggaren: review-	Details Formatted Diff Diff
Updated patch. Rebased and with fix. (77.60 KB, patch) 2013-10-15 11:05 PDT, Michael Saboff	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Michael Saboff

Comment 1 2013-10-11 08:58:22 PDT

I'll also transition cgi_op_push_name_scope and cti_push_with_scope.

Michael Saboff

Comment 2 2013-10-14 18:00:23 PDT

Created attachment 214213 [details] Patch

Mark Lam

Comment 3 2013-10-14 19:18:33 PDT

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

Comment 4 2013-10-15 08:53:11 PDT

Created attachment 214265 [details] With fixes Mark suggested

Geoffrey Garen

Comment 5 2013-10-15 09:21:44 PDT

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

Comment 6 2013-10-15 11:05:50 PDT

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 7 2013-10-15 11:25:31 PDT

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

Michael Saboff

Comment 8 2013-10-15 11:32:15 PDT

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

Note You need to log in before you can comment on or make changes to this bug.