Bug 182702 – Miscellaneous refactoring of offlineasm.

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

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/37467887>

[Mark Lam]

Created attachment 333612 [details]
propose patch.

[Mark Lam]

Created attachment 333615 [details]
proposed patch.

[Keith Miller]

Comment on attachment 333615 [details]
proposed patch.

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:58
>      elsif X86 or X86_WIN
>          subp 8, sp
>          push a1
>          push a0
> -        call function
> +        doSlowPathCall(function)

Nit: Why not just do this in the backend? 

Then you wouldn't need to split this based on architecture.

> Source/JavaScriptCore/offlineasm/instructions.rb:259
> +     "calljs",
> +     "callnative",
> +     "callslowpath",
> +     "callslowpathvoid"

These should be camel case.  I guess you have a conflict between the names of these and the macro name for callSlowPath in the offlineAsm.

I would either rename that macro to slowPathCall or rename these to something else.

[Mark Lam]

Comment on attachment 333615 [details]
proposed patch.

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:58
>> +        doSlowPathCall(function)
> 
> Nit: Why not just do this in the backend? 
> 
> Then you wouldn't need to split this based on architecture.

Because the backend is only supposed to handle CPU instructions and not calling conventions (such as the stack adjustment made for alignment here).

>> Source/JavaScriptCore/offlineasm/instructions.rb:259
>> +     "callslowpathvoid"
> 
> These should be camel case.  I guess you have a conflict between the names of these and the macro name for callSlowPath in the offlineAsm.
> 
> I would either rename that macro to slowPathCall or rename these to something else.

Note: the "memfence" instruction above isn't camel cased  as "memFence".  That said, I'll try camel casing these and see what falls out.

[Mark Lam]

Comment on attachment 333615 [details]
proposed patch.

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

>>> Source/JavaScriptCore/offlineasm/instructions.rb:259
>>> +     "callslowpathvoid"
>> 
>> These should be camel case.  I guess you have a conflict between the names of these and the macro name for callSlowPath in the offlineAsm.
>> 
>> I would either rename that macro to slowPathCall or rename these to something else.
> 
> Note: the "memfence" instruction above isn't camel cased  as "memFence".  That said, I'll try camel casing these and see what falls out.

Actually, would you mind if I do all this renaming in a follow up patch?  That will lessen the chance that it introduces a typo bug.

[Keith Miller]

Comment on attachment 333615 [details]
proposed patch.

r=me to do the rename in a follow up. please file a bug though.

[Mark Lam]

Comment on attachment 333615 [details]
proposed patch.

Thanks for the review.  I'll follow up later with a renaming patch for the requested changes.

[Mark Lam]

(In reply to Keith Miller from comment #7)
> r=me to do the rename in a follow up. please file a bug though.

Follow up patch to be worked on in https://bugs.webkit.org/show_bug.cgi?id=182708.

[Mark Lam]

Comment on attachment 333615 [details]
proposed patch.

On second thought, I might not want to introduce these macro instructions.  I'll rethink this first.

[Mark Lam]

(In reply to Mark Lam from comment #10)
> Comment on attachment 333615 [details]
> proposed patch.
> 
> On second thought, I might not want to introduce these macro instructions. 
> I'll rethink this first.

I'm going to do the refactoring in parts.  I'll remove the macro instructions part from this patch and land everything else first.

[Mark Lam]

Created attachment 333639 [details]
proposed patch.

[Mark Lam]

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