Closed Bug 1096707 Opened 5 years ago Closed 5 years ago

x86 assembler refactoring

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch optimize-subarch-checks.patch (obsolete) — Splinter Review
The following patch series refactors the x86/x64 assembler support to be more internally consistent.
Attached patch abc-immediate-operands.patch (obsolete) — Splinter Review
AT&T style puts immediates for instructions like roundss on the left.
Attached patch abc-GvEv.patch (obsolete) — Splinter Review
This patch makes several "rr" instructions use the GvEv encoding rather than the equivalent EvGv encoding, since this puts the destination on the left, which is more consistent with most of the rest of x86.
Attached patch abc-argument-order.patch (obsolete) — Splinter Review
This is a big patch which reorders the arguments in many assembler functions to consistently use AT&T style, instead of using AT&T style in the public interfaces and spew strings, and reverse order in the m_formatter functions.
Attachment #8520316 - Attachment is obsolete: true
Attachment #8520317 - Attachment is obsolete: true
Attached patch abc-GvEv.patchSplinter Review
Attachment #8520319 - Attachment is obsolete: true
Attachment #8520321 - Attachment is obsolete: true
Blocks: 1065339
Comment on attachment 8523167 [details] [diff] [review]
abc-immediate-operands.patch

Ok, I'm calling these are ready for review. These patches will help support the VEX patches in bug 1065339.

There's no functional change in this patch; it's just reordering arguments for consistency.
Attachment #8523167 - Flags: review?(benj)
Comment on attachment 8523169 [details] [diff] [review]
abc-GvEv.patch

x86 oddly has multiple ways to encode instructions like "and %eax, %ecx". This patch uses the encodings which allow _rr instructions to be encoded more consistently with how SSE instructions are encoded.
Attachment #8523169 - Flags: review?(benj)
Comment on attachment 8523170 [details] [diff] [review]
abc-argument-order.patch

This patch also has no functionality change; it's just reordering arguments in the X86InstructionFormatter class for consistency with the rest of the assembler interface.
Attachment #8523170 - Flags: review?(benj)
Comment on attachment 8523167 [details] [diff] [review]
abc-immediate-operands.patch

Review of attachment 8523167 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to update the changeset message, as not only round instructions are involved.
Attachment #8523167 - Flags: review?(benj) → review+
Comment on attachment 8523169 [details] [diff] [review]
abc-GvEv.patch

Review of attachment 8523169 [details] [diff] [review]:
-----------------------------------------------------------------

Ha, fun.
Attachment #8523169 - Flags: review?(benj) → review+
Comment on attachment 8523170 [details] [diff] [review]
abc-argument-order.patch

Review of attachment 8523170 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long review, I was worried by the fact that there doesn't seem to be a good way to check that arguments are used the right way (e.g. if you invert two int parameters, there's not an easy way to ensure that all call sites will invert their arguments as well). But I trust your work, and if this compiles and tests pass with --tbpl, and also with --no-sse4 / --no-sse3, rs=me.
Attachment #8523170 - Flags: review?(benj) → review+
You need to log in before you can comment on or make changes to this bug.