920050 - Use xor for zeroing registers on x86/x64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: xor-for-zero.patch

Following up on a comment in an earlier bug comment [0], this patch implements using xor to set integer registers to zero on x86. It's smaller and faster on modern processors.

The main point of contention is that the xor instruction clobbers the FLAGS register. This patch makes the assumption that MacroAssembler "mov" interfaces are allowed to clobber FLAGS, and comments the one place in the code where FLAGS are live over a move of an immediate value (see the comments added in MacroAssemblerX86Shared::emitSet).

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=861543#c2

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 809192 [details] [diff] [review]
xor-for-zero.patch

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

This patch does 3 things:
 - Adding xor inside the mov(ImmWord(), …) function, and fix the only location which depends on the non-reset of the CFLAGS.
 - Doing a renaming of Imm32 to ImmWord for mov arguments.
 - Replace xor(reg, reg) by mov(ImmWord(0), …)

Can you split them accordingly, and answer the following question:

::: js/src/jit/x64/Assembler-x64.h
@@ +498,5 @@
>      }
>  
>      void mov(ImmWord word, const Register &dest) {
> +        if (word.value == 0)
> +            xorl(dest, dest);

Unless I again miss the location in the documentation about the sign extension, this only applies to immediate values, right?
  xorl -> xorq ?

Comment 2

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Comment on attachment 809192 [details] [diff] [review]
> xor-for-zero.patch
> 
> Review of attachment 809192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch does 3 things:
>  - Adding xor inside the mov(ImmWord(), …) function, and fix the only
> location which depends on the non-reset of the CFLAGS.
>  - Doing a renaming of Imm32 to ImmWord for mov arguments.
>  - Replace xor(reg, reg) by mov(ImmWord(0), …)
> 
> Can you split them accordingly, and answer the following question:

Ok.

> ::: js/src/jit/x64/Assembler-x64.h
> @@ +498,5 @@
> >      }
> >  
> >      void mov(ImmWord word, const Register &dest) {
> > +        if (word.value == 0)
> > +            xorl(dest, dest);
> 
> Unless I again miss the location in the documentation about the sign
> extension, this only applies to immediate values, right?

Right. The xor trick is just for immediate zeros.

>   xorl -> xorq ?

xorl is equivalent to xorq when both operands are the same, and it has a smaller encoding. I added a comment noting this.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: xfz-mov-immword.patch

Comment 4

[Dan Gohman [:sunfish]]

Attachment: xfz-movq-store.patch

Comment 5

[Dan Gohman [:sunfish]]

Attachment: xfz-xor-for-zero.patch

Comment 6

[Dan Gohman [:sunfish]]

Attachment: xfz-cleanup-xors.patch

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 812714 [details] [diff] [review]
xfz-xor-for-zero.patch

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

::: js/src/jit/x86/Assembler-x86.h
@@ +242,5 @@
>          AsmJSAbsoluteLink link(masm.currentOffset(), imm.kind());
>          enoughMemory_ &= asmJSAbsoluteLinks_.append(link);
>      }
>      void mov(Imm32 imm, Register dest) {
> +        mov(ImmWord(imm.value), dest);

Is this function still used?

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 812715 [details] [diff] [review]
xfz-cleanup-xors.patch

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

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +90,5 @@
>          j(ConditionFromDoubleCondition(cond), label);
>      }
>  
>      void move32(const Imm32 &imm, const Register &dest) {
> +        mov(imm, dest);

nit: Add a comment that state this is safe on x64, as the result is zero-extended.

Comment 9

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Review of attachment 812714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/x86/Assembler-x86.h
> @@ +242,5 @@
> >          AsmJSAbsoluteLink link(masm.currentOffset(), imm.kind());
> >          enoughMemory_ &= asmJSAbsoluteLinks_.append(link);
> >      }
> >      void mov(Imm32 imm, Register dest) {
> > +        mov(ImmWord(imm.value), dest);
> 
> Is this function still used?

Good spot. Only by my subsequent patch, and...

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Review of attachment 812715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/shared/MacroAssembler-x86-shared.h
> @@ +90,5 @@
> >          j(ConditionFromDoubleCondition(cond), label);
> >      }
> >  
> >      void move32(const Imm32 &imm, const Register &dest) {
> > +        mov(imm, dest);
> 
> nit: Add a comment that state this is safe on x64, as the result is
> zero-extended.

... this is it. So I updated it to promote the immediate to ImmWord, which allows it to use an explicit cast to explicitly perform the zero extension. And I added a comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e31e0e258504
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f03992dc7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/734521b46dc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/922c7710220a

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e31e0e258504
https://hg.mozilla.org/mozilla-central/rev/b0f03992dc7f
https://hg.mozilla.org/mozilla-central/rev/734521b46dc9
https://hg.mozilla.org/mozilla-central/rev/922c7710220a