Bug 227857 – Add SMNEGL, UMNEGL, UMADDL, and UMSUBL for ARM64 and select this instruction in Air

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yijia Huang]

...

[Yijia Huang]

Created attachment 433282 [details]
Patch

[Yijia Huang]

Created attachment 433285 [details]
Patch

[Robin Morisset]

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

[Yijia Huang]

Created attachment 433341 [details]
Patch

[Yijia Huang]

Created attachment 433343 [details]
Patch

[Robin Morisset]

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

[Yijia Huang]

Created attachment 433346 [details]
Patch for landing

[Yijia Huang]

Created attachment 433347 [details]
Patch for landing

[Yijia Huang]

Created attachment 433348 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/80479986>