Bug 227857 - Add SMNEGL, UMNEGL, UMADDL, and UMSUBL for ARM64 and select this instruction in Air
Summary: Add SMNEGL, UMNEGL, UMADDL, and UMSUBL for ARM64 and select this instruction ...
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-10 17:08 PDT by Yijia Huang
Modified: 2021-07-12 13:52 PDT (History)
9 users (show)

See Also:


Attachments
Patch (41.51 KB, patch)
2021-07-10 17:34 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (45.33 KB, patch)
2021-07-10 23:45 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (44.57 KB, patch)
2021-07-12 12:47 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (44.57 KB, patch)
2021-07-12 12:50 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch for landing (44.57 KB, patch)
2021-07-12 13:01 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch for landing (44.58 KB, patch)
2021-07-12 13:07 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (44.58 KB, patch)
2021-07-12 13:10 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-10 17:08:07 PDT
...
Comment 1 Yijia Huang 2021-07-10 17:34:31 PDT
Created attachment 433282 [details]
Patch
Comment 2 Yijia Huang 2021-07-10 23:45:20 PDT
Created attachment 433285 [details]
Patch
Comment 3 Robin Morisset 2021-07-12 11:54:08 PDT
Comment on attachment 433285 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        UMNEGL, UMADDL, and UMSUBL) of them. In addition, this patch refactors 

You just mentioned UMADDL twice: both as something that was already implemented, and as something that was missing.

> Source/JavaScriptCore/ChangeLog:21
> +        d = -(SExt32(n) * SExt32(m)) and d = -(SExt32(n) * SExt32(m)) respectively.

The two patterns are identical. Is it on purpose, or should the latter be Zext32 each time?

> Source/JavaScriptCore/ChangeLog:24
> +        Int @0 = S/UExt32(Trunc(ArgumentReg(%x0)))

nitpick: UExt32 -> ZExt32 (I think that is what you meant here?)

> Source/JavaScriptCore/ChangeLog:47
> +        d = ZExt32(n) * ZExt32(m) + a

Maybe also mention that d = a + ZExt32(n) * ZExt32(m) is also recognized so that readers don't wonder.

> Source/JavaScriptCore/ChangeLog:50
> +        Int @0 = UExt32(Trunc(ArgumentReg(%x0)))

nit: UExt -> ZExt

> Source/JavaScriptCore/ChangeLog:77
> +        Int @0 = UExt32(Trunc(ArgumentReg(%x0)))

nit: UExt -> ZExt

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2761
> +                auto tryB3Opcode = [&] (Value* v, B3::Opcode b3Opcode) -> bool { 

It is the third instance of this same function. Maybe it could be refactored out and given a more explicit name?
Maybe something like isMergeableB3Opcode (I'm not super happy with that name, but I feel like it is already clearer than tryB3Opcode).

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:278
> +arm64: MultiplyNegSignExtend32 U:G:64, U:G:64, D:G:64

Should the first two not be U:G:32 since the instruction includes an extension from 32 to 64 bits?

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:281
> +arm64: MultiplyNegZeroExtend32 U:G:64, U:G:64, D:G:64

ditto
Comment 4 Yijia Huang 2021-07-12 12:47:57 PDT
Created attachment 433341 [details]
Patch
Comment 5 Yijia Huang 2021-07-12 12:50:26 PDT
Created attachment 433343 [details]
Patch
Comment 6 Robin Morisset 2021-07-12 12:57:40 PDT
Comment on attachment 433343 [details]
Patch

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

r=me

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2766
> +                    // ZMNEG: d = -(ZExt32(n) * ZExt32(m))

nitpick: ZMNEG -> UMNEG
Comment 7 Yijia Huang 2021-07-12 13:01:35 PDT
Created attachment 433346 [details]
Patch for landing
Comment 8 Yijia Huang 2021-07-12 13:07:36 PDT
Created attachment 433347 [details]
Patch for landing
Comment 9 Yijia Huang 2021-07-12 13:10:54 PDT
Created attachment 433348 [details]
Patch
Comment 10 EWS 2021-07-12 13:51:18 PDT
Committed r279850 (239605@main): <https://commits.webkit.org/239605@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433348 [details].
Comment 11 Radar WebKit Bug Importer 2021-07-12 13:52:20 PDT
<rdar://problem/80479986>