839582 - IonMonkey: assume high word of 64-bit registers are zero

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

The Intel manual (Vol 1, 3.4.1.1) specifies that, when the operand size is 32-bits, the result is zero-extended into the 64-bit register.  Thus, as long as we stick to 32-bit operand-sized instructions, we can assume this property for MIRType_Int32 values.  Currently, we sortof assume this isn't true and perform a truncating mov in boxValue.  However, we actually *do* make this assumption in convertUInt32ToDouble (by using cvtsq2sd).  In general, it is useful to make this assumption (viz., to avoid a truncating mov before a 64-bit base+index effective-address computation).

One place we had had a problem before was emitDoubleToInt32, where we were using cvttsd2sq, claiming to "preserve a little sanity", but I think we could just as well use cvttsd2si.  With this change, I was able to remove the truncating mov in boxValue.  To catch bugs, I put in a debug-only assertion that the high bits are zero.

Another worry I'd heard was about sign-extending 32-bit immediate movs, i.e. "movq r64, imm32".  This mov is not even present in X86Assembler, so not a problem.  To future-proof against this mistake, I added an undefined movq_i32r (which would be the canonical name) with a warning comment.

Patch passes jit tests.  (Sean, where is your [:sstangl]?)

Comment 1

[Sean Stangl [:sstangl]]

Comment on attachment 711917 [details] [diff] [review]
patch

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

Great! We didn't put effort into tracking down what was sign-extending the upper bits -- it's very nice to see that code removed.

I forgot about :sstangl! I seldom type it myself.

::: js/src/assembler/assembler/X86Assembler.h
@@ +1607,5 @@
>               (unsigned long long int)imm, nameIReg(8,dst));
>          m_formatter.oneByteOp64(OP_MOV_EAXIv, dst);
>          m_formatter.immediate64(imm);
>      }
>      

There appears to be some preexisting whitespace here: would you mind removing it?

::: js/src/ion/x64/Assembler-x64.h
@@ +541,2 @@
>      }
>      

There appears to be some preexisting whitespace here: would you mind removing it?

@@ -583,5 @@
> -    void cvttsd2sq(const FloatRegister &src, const Register &dest) {
> -        masm.cvttsd2sq_rr(src.code(), dest.code());
> -    }
> -    void cvttsd2s(const FloatRegister &src, const Register &dest) {
> -        cvttsd2sq(src, dest);

Please remove the definition for cvttsd2s() from Assembler-x86.h as well.

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +199,5 @@
>          JSValueShiftedTag tag = (JSValueShiftedTag)JSVAL_TYPE_TO_SHIFTED_TAG(type);
>          movq(ImmShiftedTag(tag), dest);
> +#ifdef DEBUG
> +        if (type == JSVAL_TYPE_INT32 || type == JSVAL_TYPE_BOOLEAN) {
> +            Label l;

This code is subtle. It would probably be easier to read if instead of "l", the Label were named "upper32BitsZeroed", which is a mouthful, but at least it communicates intent.

@@ +201,5 @@
> +#ifdef DEBUG
> +        if (type == JSVAL_TYPE_INT32 || type == JSVAL_TYPE_BOOLEAN) {
> +            Label l;
> +            cmpq(src, Imm32(UINT32_MAX));
> +            j(Assembler::BelowOrEqual, &l);

We normally avoid j() like the plague, instead preferring:
branchPtr(Assembler::BelowOrEqual, src, Imm32(UINT32_Max), &upper32BitsZeroed)

That will require definition of cmpPtr(const Operand &lhs, const Imm32 rhs), which should have a note about the sign extension.

Comment 2

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/669a28fc2724

ugh, just noticed the bad grammar... oh well, nobody but Ms2ger reads these things, right?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/669a28fc2724

Comment 4

[Jeff Walden [:Waldo]]

# HG changeset patch
# User Luke Wagner <[e-mail]>
# Date 1360371033 28800
# Node ID 669a28fc2724f9a90b14e0b06f6e89f3f7ab3d18
# Parent  63d7d58ea536e31a126eb05edcc91cbf079b40fc
Bug 839582 - IonMonkey: assume high word of 64-bit registers are zero (r=sstangl)

bad grammar

Comment 5

[Luke Wagner [:luke]]

KHAAAAAAN!