875910 - Improve x86/x64 test instruction encoding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

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.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

Comment 2

[Luke Wagner [:luke]]

So is it only 16-bit operations that are to be avoided?  The 8-bit ones are fine?

Comment 3

[Sean Stangl [:sstangl]]

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.

Comment 4

[Dan Gohman [:sunfish]]

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

[Sean Stangl [:sstangl]]

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?

Comment 6

[Dan Gohman [:sunfish]]

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

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef66f749167d

Comment 8

[Dan Gohman [:sunfish]]

Attachment: trailing whitespace cleanup

Comment 9

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d95868cd23f

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d95868cd23f