Bug 228057 - Add ARM64 EON opcode and select it in AIR
Summary: Add ARM64 EON opcode and select it in AIR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yijia Huang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-18 06:18 PDT by Yijia Huang
Modified: 2021-07-20 16:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (34.36 KB, patch)
2021-07-18 06:23 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (35.92 KB, patch)
2021-07-18 15:47 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (35.94 KB, patch)
2021-07-20 15:24 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2021-07-18 06:18:48 PDT
...
Comment 1 Yijia Huang 2021-07-18 06:23:10 PDT
Created attachment 433749 [details]
Patch
Comment 2 Yijia Huang 2021-07-18 15:47:34 PDT
Created attachment 433762 [details]
Patch
Comment 3 Saam Barati 2021-07-20 15:11:36 PDT
Comment on attachment 433762 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:53
> +        // Old optimized AIR
> +        Lshift64 %x1, amount, %x1, @4
> +        Not      %x1,    %x1,      @5
> +        Xor      %x0,    %x1, %x0, @6
> +        Ret      %x0,              @7
> +
> +        // New optimized AIR
> +        XorNotLeftShift %x0, %x1, $63, %x0, @6

Nice

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:829
> +    Air::Opcode lowerB3toAirInOpcode(B3::Opcode b3Opcode, 

I feel like we need a much more specific name here. The current one makes is not suggestive of what it's doing.

maybe "opcodeBasedOnShiftKind"? or something similar

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3144
> +            auto tryAppendEON = [&] (Value* left, Value* right) -> bool {

this looks good 👍

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:884
> +arm64: XorNotLeftShift64 U:G:64, U:G:64, U:G:64, D:G:64
> +    Tmp, Tmp, Imm, Tmp
> +
> +arm64: XorNotRightShift64 U:G:64, U:G:64, U:G:64, D:G:64
> +    Tmp, Tmp, Imm, Tmp
> +
> +arm64: XorNotUnsignedRightShift64 U:G:64, U:G:64, U:G:64, D:G:64
> +    Tmp, Tmp, Imm, Tmp

Why is the use of the Imm 64 bit instead of 32? I think 32 is more correct and consistent w/ how we do it elsewhere
Comment 4 Yijia Huang 2021-07-20 15:24:25 PDT
Created attachment 433904 [details]
Patch
Comment 5 Saam Barati 2021-07-20 16:05:15 PDT
Comment on attachment 433904 [details]
Patch

Nice. r=me
Comment 6 EWS 2021-07-20 16:20:40 PDT
Committed r280111 (239828@main): <https://commits.webkit.org/239828@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433904 [details].
Comment 7 Radar WebKit Bug Importer 2021-07-20 16:21:17 PDT
<rdar://problem/80864899>