Bug 122645 – Transition misc cti_op_* JITStubs to JIT operations

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

[Mark Lam]

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 214114 [details]
the patch.

[Michael Saboff]

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)

[Mark Lam]

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.

[Mark Lam]

Created attachment 214129 [details]
patch 2: with Michael's feedback applied.

[Michael Saboff]

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.

[Mark Lam]

Thanks for the review.  Landed in r157404: <http://trac.webkit.org/r157404>.