Bug 228057

Summary: Add ARM64 EON opcode and select it in AIR
Product: WebKit Reporter: Yijia Huang <yijia_huang>
Component: JavaScriptCoreAssignee: Yijia Huang <yijia_huang>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 228776    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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>