Bug 226849 – Air ARM64 sub32 opcode should indicate that it zero-extends its result

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

[Saam Barati]

...

[Yijia Huang]

Created attachment 431030 [details]
Patch

[Yijia Huang]

Created attachment 431034 [details]
Patch

[Yijia Huang]

Created attachment 431124 [details]
Patch

[Mark Lam]

Comment on attachment 431124 [details]
Patch

This patch needs a ChangeLog.

[Yijia Huang]

Created attachment 431127 [details]
Patch

[Mark Lam]

Comment on attachment 431127 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        * b3/air/AirOpcode.opcodes: Zero def the destination of arm64 sub32 

This description is not accurate.  You're not defining the destination register as zero.  Instead, you're indicating that the opcode will zero extend the 32-bit result to full register length in the destination.

Please also fix the title accordingly.  Also to provide context for this patch, I indicate that this is in Air opcode.  For example, something like "Air ARM64 sub32 opcode should indicate that it zero-extends its result".

[Yijia Huang]

Created attachment 431143 [details]
Patch

[Saam Barati]

Comment on attachment 431143 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        When the result of sub32 needs to be stored to a 64-sized register, 
> +        the current Air optimizer will generate an extra Move32 instruction.

I'd be more precise here. Maybe something like this:

"Sub32 was previously not saying that its result is zero defined. However, sub32 on arm64 architectures zero defines its result, so the top 32 bits are zeroed. The issue with this is what we were not eliding provably redundant zero extend operations."

> Source/JavaScriptCore/ChangeLog:19
> +        // Optimized Air IR

I'd say that this is the "Old optimized Air IR"

> Source/JavaScriptCore/ChangeLog:25
> +        To remove that redundant instruction (Move32), Air arm64 sub32 opcode 
> +        should indicate that it zero-extends its result.

"To remove that redundant instruction (Move32)" => "To remove that redundant zero extend instruction (Move32)"

> Source/JavaScriptCore/ChangeLog:26
> +

maybe also show what the new Optimized IR is here:

> Source/JavaScriptCore/ChangeLog:28
> +        * b3/air/AirOpcode.opcodes: Changed "D:G:32" to "ZD:G:32" in Air ARM64
> +        sub32 opcode.

No need for this here. We typically don't say "what" a patch does for a simple patch like this, since if you look at the file below, it already says that.

[Yijia Huang]

Created attachment 431150 [details]
Patch

[Yijia Huang]

Created attachment 431151 [details]
Patch

[Saam Barati]

Comment on attachment 431151 [details]
Patch

NIce. r=me

[EWS]

Committed r278769 (238728@main): <https://commits.webkit.org/238728@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431151 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79208100>