Bug 122645 - Transition misc cti_op_* JITStubs to JIT operations
Summary: Transition misc cti_op_* JITStubs to JIT operations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 122287
  Show dependency treegraph
 
Reported: 2013-10-11 08:28 PDT by Mark Lam
Modified: 2013-10-14 09:41 PDT (History)
5 users (show)

See Also:


Attachments
the patch. (44.34 KB, patch)
2013-10-13 14:49 PDT, Mark Lam
msaboff: review-
Details | Formatted Diff | Diff
patch 2: with Michael's feedback applied. (43.00 KB, patch)
2013-10-13 23:53 PDT, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-10-11 08:28:13 PDT
Transition these stubs to operations
    cti_op_check_has_instance
    cti_op_create_arguments
    cti_op_del_by_id
    cti_op_instanceof
    cti_to_object
    cti_op_push_activation
    cti_op_push_name_scope
    cti_op_push_with_scope
    cti_op_get_pnames
    cti_op_load_varargs
Comment 1 Mark Lam 2013-10-11 09:02:40 PDT
Michael will take care of cti_op_push_name_scope and cti_push_with_scope in https://bugs.webkit.org/show_bug.cgi?id=122617.
Comment 2 Mark Lam 2013-10-13 14:49:07 PDT
Created attachment 214114 [details]
the patch.
Comment 3 Michael Saboff 2013-10-13 23:21:34 PDT
Comment on attachment 214114 [details]
the patch.

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

> Source/JavaScriptCore/jit/CCallHelpers.h:963
> +        move(arg2, GPRInfo::argumentGPR2); // In case the incoming arg2 was in argumentGPR1.

The comment should actually read "In case arg2 is argumentGPR1".

> Source/JavaScriptCore/jit/JIT.h:541
> +        void emitGetVirtualRegister(int index, RegisterID tag, RegisterID payload);

How is this different than emitLoad(int index, RegisterID tag, RegisterID payload, RegisterID base = callFrameRegister) below?

> Source/JavaScriptCore/jit/JITInlines.h:725
> +inline void JIT::emitGetVirtualRegister(int index, RegisterID tag, RegisterID payload)
> +{
> +    emitLoadTag(index, tag);
> +    emitLoadPayload(index, payload);
> +}

Remove, this is the same as inline void JIT::emitLoad(int index, RegisterID tag, RegisterID payload, RegisterID base)
Comment 4 Mark Lam 2013-10-13 23:32:03 PDT
Comment on attachment 214114 [details]
the patch.

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

>> Source/JavaScriptCore/jit/CCallHelpers.h:963
>> +        move(arg2, GPRInfo::argumentGPR2); // In case the incoming arg2 was in argumentGPR1.
> 
> The comment should actually read "In case arg2 is argumentGPR1".

Will fix.

>> Source/JavaScriptCore/jit/JIT.h:541
>> +        void emitGetVirtualRegister(int index, RegisterID tag, RegisterID payload);
> 
> How is this different than emitLoad(int index, RegisterID tag, RegisterID payload, RegisterID base = callFrameRegister) below?

I was going to say that emitLoad() didn't check for the case where index is referring to a CodeBlock constant, but I was wrong.  I was thrown off by the tricky use of (base == callFrameRegister) to indicate that we need to go through emitLoadPayload() and emitLoadTag().  I will fix this.

>> Source/JavaScriptCore/jit/JITInlines.h:725
>> +}
> 
> Remove, this is the same as inline void JIT::emitLoad(int index, RegisterID tag, RegisterID payload, RegisterID base)

Will do.
Comment 5 Mark Lam 2013-10-13 23:53:02 PDT
Created attachment 214129 [details]
patch 2: with Michael's feedback applied.
Comment 6 Michael Saboff 2013-10-14 09:35:20 PDT
Comment on attachment 214129 [details]
patch 2: with Michael's feedback applied.

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

r+, with whitespace changes.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1047
> +

Eliminate this blank line to indicate that the links and subsequent code go together.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1064
> +

Dito.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1134
> +

Dito.
Comment 7 Mark Lam 2013-10-14 09:41:37 PDT
Thanks for the review.  Landed in r157404: <http://trac.webkit.org/r157404>.