Use xor for zeroing registers on x86/x64

RESOLVED FIXED in mozilla27

Status

()

--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla27
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 809192 [details] [diff] [review]
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
Attachment #809192 - Flags: review?(nicolas.b.pierron)
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 ?
Attachment #809192 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
Created attachment 812709 [details] [diff] [review]
xfz-mov-immword.patch
Attachment #809192 - Attachment is obsolete: true
Attachment #812709 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

5 years ago
Created attachment 812711 [details] [diff] [review]
xfz-movq-store.patch
Attachment #812711 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 5

5 years ago
Created attachment 812714 [details] [diff] [review]
xfz-xor-for-zero.patch
Attachment #812714 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 6

5 years ago
Created attachment 812715 [details] [diff] [review]
xfz-cleanup-xors.patch
Attachment #812715 - Flags: review?(nicolas.b.pierron)
Attachment #812709 - Flags: review?(nicolas.b.pierron) → review+
Attachment #812711 - Flags: review?(nicolas.b.pierron) → review+
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?
Attachment #812714 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #812715 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 9

5 years ago
(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
You need to log in before you can comment on or make changes to this bug.