Implement truncated d-to-i with a stub on x86

src/ia32/code-stubs-ia32.cc

Index: src/ia32/code-stubs-ia32.cc
diff --git a/src/ia32/code-stubs-ia32.cc b/src/ia32/code-stubs-ia32.cc
index 29a4be2140ffc629a81ab8d55b9a83a1d4a69536..b85a4198dac9c7bd8c1586f505fa07c4d4b68cf8 100644
--- a/src/ia32/code-stubs-ia32.cc
+++ b/src/ia32/code-stubs-ia32.cc
@@ -619,131 +619,141 @@ class FloatingPointHelper : public AllStatic {
 };
 
 
-// Get the integer part of a heap number.  Surprisingly, all this bit twiddling
-// is faster than using the built-in instructions on floating point registers.
-// Trashes edi and ebx.  Dest is ecx.  Source cannot be ecx or one of the
-// trashed registers.
-static void IntegerConvert(MacroAssembler* masm,
-                           Register source,
-                           bool use_sse3,
-                           Label* conversion_failure) {
-  ASSERT(!source.is(ecx) && !source.is(edi) && !source.is(ebx));
-  Label done, right_exponent, normal_exponent;
-  Register scratch = ebx;
-  Register scratch2 = edi;
-  // Get exponent word.
-  __ mov(scratch, FieldOperand(source, HeapNumber::kExponentOffset));
-  // Get exponent alone in scratch2.
-  __ mov(scratch2, scratch);
-  __ and_(scratch2, HeapNumber::kExponentMask);
-  __ shr(scratch2, HeapNumber::kExponentShift);
-  __ sub(scratch2, Immediate(HeapNumber::kExponentBias));
-  // Load ecx with zero.  We use this either for the final shift or
-  // for the answer.
-  __ xor_(ecx, ecx);
-  // If the exponent is above 83, the number contains no significant
-  // bits in the range 0..2^31, so the result is zero.
-  static const uint32_t kResultIsZeroExponent = 83;
-  __ cmp(scratch2, Immediate(kResultIsZeroExponent));
-  __ j(above, &done);
-  if (use_sse3) {
+void DoubleToIStub::Generate(MacroAssembler* masm) {
+  Register input_reg = this->source();
+  Register final_result_reg = this->destination();
+  ASSERT(is_truncating());
+
+  Label check_negative, process_64_bits, done, done_no_stash;
+
+  int double_offset = offset();
+
+  // Account for return address and saved regs if input is esp.
+  if (input_reg.is(esp)) double_offset += 3 * kPointerSize;
+
+  MemOperand mantissa_operand(MemOperand(input_reg, double_offset));
+  MemOperand exponent_operand(MemOperand(input_reg,
+                                         double_offset + kPointerSize));
+
+  Register scratch1 = ebx;
+  if (final_result_reg.is(ebx)) {
+    scratch1 = input_reg.is(edx) ? edi : edx;
+  } else if (final_result_reg.is(edx)) {
+    scratch1 = input_reg.is(ebx) ? edi : ebx;
+  } else if (input_reg.is(ebx)) {
+    scratch1 = edx;
+  }

[Yang, 2013/07/11 12:00:27]

Seems easy to introduce bugs in here.

How about something like

  Register scratch1;
  {
    Register scratch_candidates[3] = { ebx, edx, edi };
    for (int i = 0; i < 3; i++) {
      scratch1 = scratch_candidates[i];
      if (!final_result_reg.is(scratch1) && input_reg.is(scratch1)) break;
    }
  }

[danno, 2013/07/11 16:00:23]

Done.

+  Register result_reg = final_result_reg.is(ecx) ? eax : final_result_reg;
+  Register save_reg = final_result_reg.is(ecx) ? eax : ecx;

[Yang, 2013/07/11 12:00:27]

why do we need to spill eax if the result is expected in ecx? Just so that the
esp offset is 3 stack slots?

[danno, 2013/07/11 16:00:23]

No, it's because we use eax above to calculate the result above if the the real
result is ecx, and part of the contract is that we don't clobber registers. I'll
comment.

+  __ push(scratch1);
+  __ push(save_reg);
+
+  bool stash_exponent_copy = !input_reg.is(esp);
+  if (!input_reg.is(ecx)) {
+    __ mov(ecx, exponent_operand);
+    if (stash_exponent_copy) __ push(ecx);
+  }
+  __ mov(scratch1, mantissa_operand);
+  if (CpuFeatures::IsSupported(SSE3)) {
     CpuFeatureScope scope(masm, SSE3);
-    // Check whether the exponent is too big for a 64 bit signed integer.
-    static const uint32_t kTooBigExponent = 63;
-    __ cmp(scratch2, Immediate(kTooBigExponent));
-    __ j(greater_equal, conversion_failure);
     // Load x87 register with heap number.
-    __ fld_d(FieldOperand(source, HeapNumber::kValueOffset));
-    // Reserve space for 64 bit answer.
-    __ sub(esp, Immediate(sizeof(uint64_t)));  // Nolint.
+    __ fld_d(mantissa_operand);
+  }
+  if (input_reg.is(ecx)) {
+    __ mov(ecx, exponent_operand);
+    if (stash_exponent_copy) __ push(ecx);
+  }

[Yang, 2013/07/11 12:00:27]

At this point, we have overwritten ecx regardless what input_reg is. Couldn't we
just always load the mantissa onto the FP stack first and then overwrite ecx
with the exponent?

Unless you expect performance difference by loading the exponent earlier, in
which case, ignore this comment.

[danno, 2013/07/11 16:00:23]

Done.

+  __ shr(ecx, HeapNumber::kExponentShift);
+  __ and_(ecx,
+          Immediate(HeapNumber::kExponentMask >> HeapNumber::kExponentShift));

[Yang, 2013/07/11 12:00:27]

Looks like we could first mask and then shift, like we do elsewhere, so that the
mask doesn't have to be shifted. Easier to read imo.

[danno, 2013/07/11 16:00:23]

Done.

+  __ lea(result_reg, MemOperand(ecx, -HeapNumber::kExponentBias));

[Yang, 2013/07/11 12:00:27]

As far as I can see the value in result_reg is not used later on. We could just
compare ecx to (HeapNumber::kExponentBias + HeapNumber::kMantissaBits) directly,
without lea.

Or even use sub instead of cmp, which stores the result in ecx for later and
also sets the flags for the following conditional jump. In line 702-704 we would
then simply neg(ecx).

[danno, 2013/07/11 16:00:23]

This is actually a different test. The cmp and below branch tests a range of
values. You can't do that if you simply do the subtract and check for less that
zero.

+  __ cmp(result_reg, Immediate(HeapNumber::kMantissaBits));
+  __ j(below, &process_64_bits);
+
+  // Result is entirely in lower 32-bits of mantissa
+  int delta = HeapNumber::kExponentBias + Double::kPhysicalSignificandSize;
+  if (CpuFeatures::IsSupported(SSE3)) {
+    __ fstp(0);
+  }
+  __ sub(ecx, Immediate(delta));
+  __ mov(result_reg, Immediate(0));

[Yang, 2013/07/11 12:00:27]

Use Set instead of mov to use xor, which is shorter. Or just use xor inline.

[danno, 2013/07/11 16:00:23]

Done.

+  __ cmp(ecx, Immediate(31));
+  __ j(above, &done);
+  __ shl_cl(scratch1);
+  __ jmp(&check_negative);
+
+  __ bind(&process_64_bits);
+  if (CpuFeatures::IsSupported(SSE3)) {
+    CpuFeatureScope scope(masm, SSE3);
+    if (stash_exponent_copy) {
+      // Already a copy of the mantissa on the stack, overwrite it.

[Yang, 2013/07/11 12:00:27]

Comment seems wrong. We pushed a copy of the exponent onto the stack, not of the
mantissa.

We also should, at some point, add a STATIC_ASSERT(kDoubleSize == 2 *
kPointerSize), since we do rely on that.

[danno, 2013/07/11 16:00:23]

Done.

+      __ sub(esp, Immediate(kDoubleSize / 2));
+    } else {
+      // Reserve space for 64 bit answer.
+      __ sub(esp, Immediate(kDoubleSize));  // Nolint.
+    }
     // Do conversion, which cannot fail because we checked the exponent.
     __ fisttp_d(Operand(esp, 0));
-    __ mov(ecx, Operand(esp, 0));  // Load low word of answer into ecx.
+    __ mov(result_reg, Operand(esp, 0));  // Load low word of answer as result
     __ add(esp, Immediate(sizeof(uint64_t)));  // Nolint.

[Yang, 2013/07/11 12:00:27]

We should use kDoubleSize for consistency. Also, do we really need Nolint?

[danno, 2013/07/11 16:00:23]

Done.

+    __ jmp(&done_no_stash);
   } else {
-    // Check whether the exponent matches a 32 bit signed int that cannot be
-    // represented by a Smi.  A non-smi 32 bit integer is 1.xxx * 2^30 so the
-    // exponent is 30 (biased).  This is the exponent that we are fastest at and
-    // also the highest exponent we can handle here.
-    const uint32_t non_smi_exponent = 30;
-    __ cmp(scratch2, Immediate(non_smi_exponent));
-    // If we have a match of the int32-but-not-Smi exponent then skip some
-    // logic.
-    __ j(equal, &right_exponent, Label::kNear);
-    // If the exponent is higher than that then go to slow case.  This catches
-    // numbers that don't fit in a signed int32, infinities and NaNs.
-    __ j(less, &normal_exponent, Label::kNear);
-
-    {
-      // Handle a big exponent.  The only reason we have this code is that the
-      // >>> operator has a tendency to generate numbers with an exponent of 31.
-      const uint32_t big_non_smi_exponent = 31;
-      __ cmp(scratch2, Immediate(big_non_smi_exponent));
-      __ j(not_equal, conversion_failure);
-      // We have the big exponent, typically from >>>.  This means the number is
-      // in the range 2^31 to 2^32 - 1.  Get the top bits of the mantissa.
-      __ mov(scratch2, scratch);
-      __ and_(scratch2, HeapNumber::kMantissaMask);
-      // Put back the implicit 1.
-      __ or_(scratch2, 1 << HeapNumber::kExponentShift);
-      // Shift up the mantissa bits to take up the space the exponent used to
-      // take. We just orred in the implicit bit so that took care of one and
-      // we want to use the full unsigned range so we subtract 1 bit from the
-      // shift distance.
-      const int big_shift_distance = HeapNumber::kNonMantissaBitsInTopWord - 1;
-      __ shl(scratch2, big_shift_distance);
-      // Get the second half of the double.
-      __ mov(ecx, FieldOperand(source, HeapNumber::kMantissaOffset));
-      // Shift down 21 bits to get the most significant 11 bits or the low
-      // mantissa word.
-      __ shr(ecx, 32 - big_shift_distance);
-      __ or_(ecx, scratch2);
-      // We have the answer in ecx, but we may need to negate it.
-      __ test(scratch, scratch);
-      __ j(positive, &done, Label::kNear);
-      __ neg(ecx);
-      __ jmp(&done, Label::kNear);
+    // Result must be extracted from shifted 32-bit mantissa
+    __ mov(result_reg, Immediate(delta));
+    __ sub(result_reg, ecx);
+    __ mov(ecx, result_reg);

[Yang, 2013/07/11 12:00:27]

sub(ecx, Immediate(delta));
neg(ecx);

should do the same?

Alternatively, the sub has already been done as described above, so we just need
a neg.

[danno, 2013/07/11 16:00:23]

Done.

+    if (stash_exponent_copy) {
+      __ mov(result_reg, MemOperand(esp, 0));
+    } else {
+      __ mov(result_reg, exponent_operand);
+    }
+    __ and_(result_reg,
+            Immediate(static_cast<uint32_t>(Double::kSignificandMask >> 32)));
+    __ add(result_reg,
+           Immediate(static_cast<uint32_t>(Double::kHiddenBit >> 32)));
+    __ shrd(result_reg, scratch1);
+    __ shr_cl(result_reg);
+    __ test(ecx, Immediate(32));
+    if (CpuFeatures::IsSupported(CMOV)) {
+      CpuFeatureScope use_cmov(masm, CMOV);
+      __ cmov(not_equal, scratch1, result_reg);
+    } else {
+      Label skip_mov;
+      __ j(equal, &skip_mov);

[Yang, 2013/07/11 12:00:27]

mark as near jump.

[danno, 2013/07/11 16:00:23]

Done.

+      __ mov(scratch1, result_reg);
+      __ bind(&skip_mov);
     }
+  }
 
-    __ bind(&normal_exponent);
-    // Exponent word in scratch, exponent in scratch2. Zero in ecx.
-    // We know that 0 <= exponent < 30.
-    __ mov(ecx, Immediate(30));
-    __ sub(ecx, scratch2);
-
-    __ bind(&right_exponent);
-    // Here ecx is the shift, scratch is the exponent word.
-    // Get the top bits of the mantissa.
-    __ and_(scratch, HeapNumber::kMantissaMask);
-    // Put back the implicit 1.
-    __ or_(scratch, 1 << HeapNumber::kExponentShift);
-    // Shift up the mantissa bits to take up the space the exponent used to
-    // take. We have kExponentShift + 1 significant bits int he low end of the
-    // word.  Shift them to the top bits.
-    const int shift_distance = HeapNumber::kNonMantissaBitsInTopWord - 2;
-    __ shl(scratch, shift_distance);
-    // Get the second half of the double. For some exponents we don't
-    // actually need this because the bits get shifted out again, but
-    // it's probably slower to test than just to do it.
-    __ mov(scratch2, FieldOperand(source, HeapNumber::kMantissaOffset));
-    // Shift down 22 bits to get the most significant 10 bits or the low
-    // mantissa word.
-    __ shr(scratch2, 32 - shift_distance);
-    __ or_(scratch2, scratch);
-    // Move down according to the exponent.
-    __ shr_cl(scratch2);
-    // Now the unsigned answer is in scratch2.  We need to move it to ecx and
-    // we may need to fix the sign.
-    Label negative;
-    __ xor_(ecx, ecx);
-    __ cmp(ecx, FieldOperand(source, HeapNumber::kExponentOffset));
-    __ j(greater, &negative, Label::kNear);
-    __ mov(ecx, scratch2);
-    __ jmp(&done, Label::kNear);
-    __ bind(&negative);
-    __ sub(ecx, scratch2);
+  // If the double was negative, negate the integer result.
+  __ bind(&check_negative);
+  __ mov(result_reg, scratch1);
+  __ neg(result_reg);
+  if (stash_exponent_copy) {
+    __ cmp(MemOperand(esp, 0), Immediate(0));
+  } else {
+    __ cmp(exponent_operand, Immediate(0));
   }
+  if (CpuFeatures::IsSupported(CMOV)) {
+    CpuFeatureScope use_cmov(masm, CMOV);
+    __ cmov(greater, result_reg, scratch1);
+  } else {
+    Label skip_mov;
+    __ j(less_equal, &skip_mov);

[Yang, 2013/07/11 12:00:27]

mark as near jump

[danno, 2013/07/11 16:00:23]

Done.

+    __ mov(result_reg, scratch1);
+    __ bind(&skip_mov);
+  }
+
+  // Restore registers
   __ bind(&done);
+  if (stash_exponent_copy) {
+    __ add(esp, Immediate(kDoubleSize / 2));
+  }
+  __ bind(&done_no_stash);
+  if (final_result_reg.is(ecx)) __ mov(ecx, save_reg);

[Yang, 2013/07/11 12:00:27]

I think something like

if (!final_result_reg.is(result_reg)) {
  ASSERT(final_result_reg.is(ecx));
  __ mov(final_result_reg, result_reg);
}

is easier to understand.

[danno, 2013/07/11 16:00:23]

Done.

+  __ pop(save_reg);
+  __ pop(scratch1);
+  __ ret(0);
 }
 
 
@@ -970,7 +980,8 @@ void UnaryOpStub::GenerateHeapNumberCodeBitNot(MacroAssembler* masm,
   __ j(not_equal, slow);
 
   // Convert the heap number in eax to an untagged integer in ecx.
-  IntegerConvert(masm, eax, CpuFeatures::IsSupported(SSE3), slow);
+  DoubleToIStub stub(eax, ecx, HeapNumber::kValueOffset - kSmiTagSize, true);

[Yang, 2013/07/11 12:00:27]

I think you want to use (HeapNumber::kValueOffset - kHeapObjectTag). Even though
both equal to 1.

[danno, 2013/07/11 16:00:23]

Done.

+  __ call(stub.GetCode(masm->isolate()), RelocInfo::CODE_TARGET);
 
   // Do the bitwise operation and check if the result fits in a smi.
   Label try_float;
@@ -1002,9 +1013,9 @@ void UnaryOpStub::GenerateHeapNumberCodeBitNot(MacroAssembler* masm,
       // New HeapNumber is in eax.
       __ pop(edx);
     }
-    // IntegerConvert uses ebx and edi as scratch registers.
-    // This conversion won't go slow-case.
-    IntegerConvert(masm, edx, CpuFeatures::IsSupported(SSE3), slow);
+    DoubleToIStub stub(edx, ecx, HeapNumber::kValueOffset - kSmiTagSize, true);

[Yang, 2013/07/11 12:00:27]

Ditto.

[danno, 2013/07/11 16:00:23]

Done.

+    __ call(stub.GetCode(masm->isolate()), RelocInfo::CODE_TARGET);
+
     __ not_(ecx);
 
     __ bind(&heapnumber_allocated);
@@ -2683,7 +2694,8 @@ void FloatingPointHelper::LoadUnknownsAsIntegers(
     CpuFeatureScope use_sse2(masm, SSE2);
     ConvertHeapNumberToInt32(masm, edx, conversion_failure);
   } else {
-    IntegerConvert(masm, edx, use_sse3, conversion_failure);
+    DoubleToIStub stub(edx, ecx, HeapNumber::kValueOffset - kSmiTagSize, true);

[Yang, 2013/07/11 12:00:27]

Ditto.

[danno, 2013/07/11 16:00:23]

Done.

+    __ call(stub.GetCode(masm->isolate()), RelocInfo::CODE_TARGET);
   }
   __ mov(edx, ecx);
 
@@ -2718,7 +2730,8 @@ void FloatingPointHelper::LoadUnknownsAsIntegers(
     CpuFeatureScope use_sse2(masm, SSE2);
     ConvertHeapNumberToInt32(masm, eax, conversion_failure);
   } else {
-    IntegerConvert(masm, eax, use_sse3, conversion_failure);
+    DoubleToIStub stub(eax, ecx, HeapNumber::kValueOffset - kSmiTagSize, true);

[Yang, 2013/07/11 12:00:27]

Ditto.

[danno, 2013/07/11 16:00:23]

Done.

+    __ call(stub.GetCode(masm->isolate()), RelocInfo::CODE_TARGET);
   }
 
   __ bind(&done);