Bug 237624 - Rename LLInt macros using return to destination
Summary: Rename LLInt macros using return to destination
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-08 15:24 PST by Keith Miller
Modified: 2022-03-15 16:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (50.68 KB, patch)
2022-03-08 15:28 PST, Keith Miller
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (84.77 KB, patch)
2022-03-08 15:32 PST, Keith Miller
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (84.44 KB, patch)
2022-03-08 15:37 PST, Keith Miller
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (84.79 KB, patch)
2022-03-08 15:41 PST, Keith Miller
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (84.30 KB, patch)
2022-03-08 15:47 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (83.74 KB, patch)
2022-03-08 16:01 PST, Keith Miller
mark.lam: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2022-03-08 15:24:47 PST
Rename LLInt macros using return to destination
Comment 1 Keith Miller 2022-03-08 15:28:02 PST
Created attachment 454160 [details]
Patch
Comment 2 Keith Miller 2022-03-08 15:32:42 PST
Created attachment 454161 [details]
Patch
Comment 3 Keith Miller 2022-03-08 15:37:18 PST
Created attachment 454162 [details]
Patch
Comment 4 Keith Miller 2022-03-08 15:41:38 PST
Created attachment 454163 [details]
Patch
Comment 5 Keith Miller 2022-03-08 15:47:35 PST
Created attachment 454164 [details]
Patch
Comment 6 Mark Lam 2022-03-08 15:49:33 PST
Comment on attachment 454161 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:832
> +llintOpWithsetDestinationAndDispatch(op_argument_count, OpArgumentCount, macro (size, get, dispatch, setDestinationAndDispatch)

This should be llintOpWithDestination, and same at all the place below.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:839
> +llintOpWithsetDestinationAndDispatch(op_get_scope, OpGetScope, macro (size, get, dispatch, setDestinationAndDispatch)

llintOpWithSet...

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:873
> +llintOpWithsetDestinationAndDispatch(op_mov, OpMov, macro (size, get, dispatch, setDestinationAndDispatch)

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:880
> +llintOpWithsetDestinationAndDispatch(op_not, OpNot, macro (size, get, dispatch, setDestinationAndDispatch)

Ditto ... and all the place below too.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:976
> +    llintOpWithsetDestinationAndDispatch(op_%opcodeName%, opcodeStruct, macro (size, get, dispatch, setDestinationAndDispatch)

Ditto

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1083
> +llintOpWithsetDestinationAndDispatch(op_to_string, OpToString, macro (size, get, dispatch, setDestinationAndDispatch)

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2366
> +    dosetDestinationAndDispatch()

Wrong search and replace: should be doReturn?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2370
> +llintOpWithsetDestinationAndDispatch(op_to_primitive, OpToPrimitive, macro (size, get, dispatch, setDestinationAndDispatch)

llintOpWithDestination

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2384
> +llintOpWithsetDestinationAndDispatch(op_to_property_key, OpToPropertyKey, macro (size, get, dispatch, setDestinationAndDispatch)

llintOpWithDestination

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2450
> +    dosetDestinationAndDispatch()

Wrong search and replace: should be doReturn?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2915
> +llintOpWithsetDestinationAndDispatch(op_get_parent_scope, OpGetParentScope, macro (size, get, dispatch, setDestinationAndDispatch)

llintOpWithDestination

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2979
> +llintOpWithsetDestinationAndDispatch(op_get_rest_length, OpGetRestLength, macro (size, get, dispatch, setDestinationAndDispatch)

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:3201
> +    dosetDestinationAndDispatch()

Wrong search and replace: should be doReturn?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2479
> -    # prepareCall in LLInt does not untag return address. So we need to untag that in the trampoline separately.
> +    # prepareCall in LLInt does not untag setDestinationAndDispatch address. So we need to untag that in the trampoline separately.

Wrong search replace?  Should this one really be "return address"?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2557
> -            # prepareCall in LLInt does not untag return address. So we need to untag that in the trampoline separately.
> +            # prepareCall in LLInt does not untag setDestinationAndDispatch address. So we need to untag that in the trampoline separately.

Ditto: Wrong search replace?
Comment 7 Keith Miller 2022-03-08 15:58:37 PST
Comment on attachment 454164 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:-1023
> -        # return result;

Will revert

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:-2479
> -    # prepareCall in LLInt does not untag return address. So we need to untag that in the trampoline separately.

Will revert
Comment 8 Keith Miller 2022-03-08 16:01:42 PST
Created attachment 454168 [details]
Patch
Comment 9 Mark Lam 2022-03-08 22:38:03 PST
Comment on attachment 454168 [details]
Patch

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

r=me.  This patch is huge, and it's very difficult to review this by eyeballing, and be confident that it was done right.  It's too easy to miss something, and perhaps introduce a subtle change.  Just for a sanity check (if you haven't already done so), please generate a LLIntAssembly.h before and after your patch, and diff the 2 LLIntAssembly.h files.  If there are no errors, they should be identical.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:529
>      llintOpWithMetadata(opcodeName, opcodeStruct, macro(size, get, dispatch, metadata, return)

/return/setDestinationAndDispatch/ to be consistent.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2557
> +            # prepareCall in LLInt does not untag setDestinationAndDispatch address. So we need to untag that in the trampoline separately.

/setDestinationAndDispatch address/the return address/

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2795
> -    macro returnConstantScope()
> +    macro setDestinationAndDispatchConstantScope()

In this case, `returnConstantScope` would be a better name if not for the fact that there's no actual "return" operation.  How about `setDestinationAndDispatchWithConstantScope` instead?  `setDestinationAndDispatchConstantScope` just doesn't read like English.
Comment 10 Radar WebKit Bug Importer 2022-03-15 16:25:29 PDT
<rdar://problem/90337195>