Bug 182702 - Miscellaneous refactoring of offlineasm.
Summary: Miscellaneous refactoring of offlineasm.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 182708
  Show dependency treegraph
 
Reported: 2018-02-12 11:49 PST by Mark Lam
Modified: 2018-02-12 15:44 PST (History)
9 users (show)

See Also:


Attachments
propose patch. (26.54 KB, patch)
2018-02-12 12:02 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (26.59 KB, patch)
2018-02-12 12:26 PST, Mark Lam
keith_miller: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
proposed patch. (9.81 KB, patch)
2018-02-12 15:29 PST, Mark Lam
fpizlo: 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 2018-02-12 11:49:57 PST
Patch coming.
Comment 1 Radar WebKit Bug Importer 2018-02-12 11:50:41 PST
<rdar://problem/37467887>
Comment 2 Mark Lam 2018-02-12 12:02:15 PST
Created attachment 333612 [details]
propose patch.
Comment 3 Mark Lam 2018-02-12 12:26:04 PST
Created attachment 333615 [details]
proposed patch.
Comment 4 Keith Miller 2018-02-12 13:03:27 PST
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.
Comment 5 Mark Lam 2018-02-12 14:17:37 PST
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.
Comment 6 Mark Lam 2018-02-12 14:33:43 PST
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.
Comment 7 Keith Miller 2018-02-12 14:35:26 PST
Comment on attachment 333615 [details]
proposed patch.

r=me to do the rename in a follow up. please file a bug though.
Comment 8 Mark Lam 2018-02-12 14:36:12 PST
Comment on attachment 333615 [details]
proposed patch.

Thanks for the review.  I'll follow up later with a renaming patch for the requested changes.
Comment 9 Mark Lam 2018-02-12 14:38:36 PST
(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.
Comment 10 Mark Lam 2018-02-12 15:05:36 PST
Comment on attachment 333615 [details]
proposed patch.

On second thought, I might not want to introduce these macro instructions.  I'll rethink this first.
Comment 11 Mark Lam 2018-02-12 15:12:57 PST
(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.
Comment 12 Mark Lam 2018-02-12 15:29:38 PST
Created attachment 333639 [details]
proposed patch.
Comment 13 Mark Lam 2018-02-12 15:44:12 PST
Thanks for the review.  Landed in r228402: <http://trac.webkit.org/r228402>.