Summary: | Air ARM64 sub32 opcode should indicate that it zero-extends its result | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yijia Huang <yijia_huang> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, tzagallo, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Saam Barati
2021-06-09 13:43:10 PDT
Created attachment 431030 [details]
Patch
Created attachment 431034 [details]
Patch
Created attachment 431124 [details]
Patch
Comment on attachment 431124 [details]
Patch
This patch needs a ChangeLog.
Created attachment 431127 [details]
Patch
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". Created attachment 431143 [details]
Patch
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. Created attachment 431150 [details]
Patch
Created attachment 431151 [details]
Patch
Comment on attachment 431151 [details]
Patch
NIce. r=me
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]. |