Closed
Bug 875910
Opened 11 years ago
Closed 11 years ago
Improve x86/x64 test instruction encoding
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: sunfish, Unassigned)
Details
Attachments
(2 files)
4.17 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
23.88 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
Instructions like testq $0x8, %rax can be rewritten to testb $0x8, %al, which is a smaller instruction due to using a smaller immediate field. cmpq $0, %rax can be rewritten to testq %rax, %rax, which is smaller.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #753924 -
Flags: review?(sstangl)
Comment 2•11 years ago
|
||
So is it only 16-bit operations that are to be avoided? The 8-bit ones are fine?
Comment 3•11 years ago
|
||
I attempted to time the cost of |testb| versus |testq|. Given the following code: > section .text > global _start > _start: > mov rdi, dword 0x100000 > xor rcx, rcx > > _loop: > times 1000 test bx, 0x8 > add rcx, dword 1 > cmp rcx, rdi > jne _loop > > _exit: > xor rdi,rdi > mov rax, dword 0x3c > syscall and a version that replaces "test bx, 0x8" with "test rbx, 0x8", the |testb| version runs in 900ms, while the |testq| version runs in 100ms. Based on the execution time of |testb|, it sounds like making this change might not be advantageous.
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 4•11 years ago
|
||
Luke: Yes, it's generally only the 16-bit operations that are to be avoided. Sean: "test bx, 0x8", is |testw|, the 16-bit form. My patch doesn't use the 16-bit form. With your testcase on a Sandy Bridge, testw is slow, while testb, testl, and testq are all fast.
Comment 5•11 years ago
|
||
Comment on attachment 753924 [details] [diff] [review] a proposed fix Review of attachment 753924 [details] [diff] [review]: ----------------------------------------------------------------- You're right -- 8-bit test operations appear to be around the same speed as 32-bit test operations with smaller code size. Nice, I had no idea! ::: js/src/assembler/assembler/X86Assembler.h @@ +1278,5 @@ > spew("testb %s, %s", > nameIReg(1,src), nameIReg(1,dst)); > m_formatter.oneByteOp(OP_TEST_EbGb, src, dst); > } > There's some preexisting trailing whitespace here, would you mind removing it?
Attachment #753924 -
Flags: review?(sstangl) → review+
Reporter | ||
Comment 6•11 years ago
|
||
I'll submit a separate patch to clean up the preexisting whitespace. I checked in the reviewed patch here: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef66f749167d
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ef66f749167d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter | ||
Comment 8•11 years ago
|
||
Attachment #760506 -
Flags: review?(sstangl)
Updated•11 years ago
|
Attachment #760506 -
Flags: review?(sstangl) → review+
Reporter | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d95868cd23f
You need to log in
before you can comment on or make changes to this bug.
Description
•