Support external arrays in Crankshaft

src/ia32/lithium-codegen-ia32.cc

Index: src/ia32/lithium-codegen-ia32.cc
diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc
index a3c0990e676089f3d6a4c266508c16dbd34f9469..4302f2636d42d44279fdcadc2b72cacd75e37f75 100644
--- a/src/ia32/lithium-codegen-ia32.cc
+++ b/src/ia32/lithium-codegen-ia32.cc
@@ -2183,11 +2183,16 @@ void LCodeGen::DoLoadElements(LLoadElements* instr) {
            Immediate(factory()->fixed_array_map()));
     __ j(equal, &done);
     __ cmp(FieldOperand(result, HeapObject::kMapOffset),
-           Immediate(factory()->external_pixel_array_map()));
-    __ j(equal, &done);
-    __ cmp(FieldOperand(result, HeapObject::kMapOffset),
            Immediate(factory()->fixed_cow_array_map()));
-    __ Check(equal, "Check for fast elements or pixel array failed.");
+    __ j(equal, &done);
+    Register temp((result.is(eax)) ? ebx : eax);

[Mads Ager (chromium), 2011/03/24 11:31:20]

Does it hurt anything to just let the register allocator deal with this and
allocate an extra temp register? Then we can avoid the push and pop here and let
the register allocator deal with spilling. On the other hand it will put more
pressure on the register allocator which could lead to extra spills where none
are needed now. We should see if we can measure a difference. The alternative is
to have a special load elements instruction when our type feedback says that we
are external arrays. That might be the cleanest way.

[danno, 2011/03/24 13:18:38]

Since this is only for debug code to verify/assert that the elements are an
external array, I didn't want to put unnecessary pressure on the register
allocator and wanted to keep the debug check very localized. 

On 2011/03/24 11:31:20, Mads Ager wrote:

[fschneider, 2011/03/24 13:26:11]

No worry :) It's only when FLAG_debug_code is on. So we should not allocate a
temp.

+    __ push(temp);
+    __ mov(temp, FieldOperand(result, HeapObject::kMapOffset));
+    __ movzx_b(temp, FieldOperand(temp, Map::kInstanceTypeOffset));
+    __ sub(Operand(temp), Immediate(FIRST_EXTERNAL_ARRAY_TYPE));
+    __ cmp(Operand(temp), Immediate(kExternalArrayTypeCount));
+    __ pop(temp);
+    __ Check(below, "Check for fast elements or pixel array failed.");
     __ bind(&done);
   }
 }
@@ -2235,14 +2240,50 @@ void LCodeGen::DoLoadKeyedFastElement(LLoadKeyedFastElement* instr) {
 }
 
 
-void LCodeGen::DoLoadPixelArrayElement(LLoadPixelArrayElement* instr) {
+void LCodeGen::DoLoadKeyedSpecializedArrayElement(
+    LLoadKeyedSpecializedArrayElement* instr) {
   Register external_pointer = ToRegister(instr->external_pointer());
   Register key = ToRegister(instr->key());
-  Register result = ToRegister(instr->result());
-  ASSERT(result.is(external_pointer));
-
-  // Load the result.
-  __ movzx_b(result, Operand(external_pointer, key, times_1, 0));
+  ExternalArrayType array_type = instr->array_type();
+  switch (array_type) {
+    case kExternalByteArray:
+      __ movsx_b(ToRegister(instr->result()),

[Kevin Millikin (Chromium), 2011/03/24 11:21:00]

I think if you named ToRegister(instr->result()) with some suitably short name,
outised the switch, then a lot of these cases would fit on one line.

[danno, 2011/03/24 13:18:38]

The problem is that the ToRegister asserts in the case that it's a double, so
ToRegister can only happen in the cases where it's an int return value. I could
introduce a temp to store result in every switch case, but that would mean
bracketing each case with {} to create a separate scope, which doesn't look
pretty to me, either. In any case, I'm happy to go with whichever variant you
prefer that works. 
On 2011/03/24 11:21:00, Kevin Millikin wrote:

[Kevin Millikin (Chromium), 2011/03/24 13:29:40]

I didn't think of the ToDoubleRegister case.  You could go with:

if (array_type == kExternalFloatArray) {
  // Float array case.
} else {
  Register res = ToRegister(instr->result());
  switch (array_type) {
    // ...
    case kExternalFloatArray:
      UNREACHABLE();
      break;
  }
}

Or leave it as it is if you prefer.

+                 Operand(external_pointer, key, times_1, 0));
+      break;
+    case kExternalUnsignedByteArray:
+    case kExternalPixelArray:
+      __ movzx_b(ToRegister(instr->result()),
+                 Operand(external_pointer, key, times_1, 0));
+      break;
+    case kExternalShortArray:
+      __ movsx_w(ToRegister(instr->result()),
+                 Operand(external_pointer, key, times_2, 0));
+      break;
+    case kExternalUnsignedShortArray:
+      __ movzx_w(ToRegister(instr->result()),
+                 Operand(external_pointer, key, times_2, 0));
+      break;
+    case kExternalIntArray:
+      __ mov(ToRegister(instr->result()),
+             Operand(external_pointer, key, times_4, 0));
+      break;
+    case kExternalUnsignedIntArray: {
+       Register result(ToRegister(instr->result()));

[Kevin Millikin (Chromium), 2011/03/24 11:21:00]

Indentation looks off a bit.

[danno, 2011/03/24 13:18:38]

Not sure what you mean, I introduced a separate scope to hold result, which
suggests the two space additional indent to me.
 On 2011/03/24 11:21:00, Kevin Millikin wrote:

[Kevin Millikin (Chromium), 2011/03/24 13:29:40]

"Register" looks indented by 3 (lines up with the "e" in "case").

+      __ mov(result, Operand(external_pointer, key, times_4, 0));
+      __ cmp(Operand(result), Immediate(0x80000000));

[Kevin Millikin (Chromium), 2011/03/24 11:21:00]

Alternatively (seems more idiomatic to me for some reason):

__ test(result, Immediate(0x80000000));
DeoptimizeIf(not_zero, instr->environment());

[danno, 2011/03/24 13:18:38]

Done.

[Lasse Reichstein, 2011/03/24 22:09:02]

Or use:
  __ test(result, Operand(result));
  DeoptimizeIf(negative, instr->environment());
It avoids the 32-bit immediate.

+      // TODO(danno): we could be more clever here, perhaps having a special
+      // version of the stub that detects if the overflow case actually happens,
+      // and generate code that returns a double rather than int.
+      DeoptimizeIf(above_equal, instr->environment());
+      break;
+    }
+    case kExternalFloatArray: {
+      XMMRegister result(ToDoubleRegister(instr->result()));
+      __ movss(result, Operand(external_pointer, key, times_4, 0));
+      __ cvtss2sd(result, result);
+      break;
+    }
+  }
 }
 
 
@@ -2867,22 +2908,47 @@ void LCodeGen::DoBoundsCheck(LBoundsCheck* instr) {
 }
 
 
-void LCodeGen::DoStorePixelArrayElement(LStorePixelArrayElement* instr) {
+void LCodeGen::DoStoreKeyedSpecializedArrayElement(
+    LStoreKeyedSpecializedArrayElement* instr) {
   Register external_pointer = ToRegister(instr->external_pointer());
   Register key = ToRegister(instr->key());
-  Register value = ToRegister(instr->value());
-  ASSERT(ToRegister(instr->TempAt(0)).is(eax));
 
-  __ mov(eax, value);
-  {  // Clamp the value to [0..255].
-    NearLabel done;
-    __ test(eax, Immediate(0xFFFFFF00));
-    __ j(zero, &done);
-    __ setcc(negative, eax);  // 1 if negative, 0 if positive.
-    __ dec_b(eax);  // 0 if negative, 255 if positive.
-    __ bind(&done);
+  switch (instr->array_type()) {
+    case kExternalPixelArray:
+      {  // Clamp the value to [0..255].
+        ASSERT(ToRegister(instr->TempAt(0)).is(eax));

[Mads Ager (chromium), 2011/03/24 11:31:20]

Instead of having this assert, let's just do

Register temp = ToRegister(instr->TempAt(0));

?

[danno, 2011/03/24 13:18:38]

Actually, I think this assert it important, since we forced the register
allocator to give us eax (an arbitrary byte register) so that the dec_b will
succeed below. I've changed the code to be more readable and added a comment.

+        __ mov(eax, ToRegister(instr->value()));
+        NearLabel done;
+        __ test(eax, Immediate(0xFFFFFF00));
+        __ j(zero, &done);
+        __ setcc(negative, eax);  // 1 if negative, 0 if positive.
+        __ dec_b(eax);  // 0 if negative, 255 if positive.
+        __ bind(&done);
+        __ mov_b(Operand(external_pointer, key, times_1, 0), eax);
+      }
+      break;
+    case kExternalByteArray:
+    case kExternalUnsignedByteArray:
+      __ mov_b(Operand(external_pointer, key, times_1, 0),
+               ToRegister(instr->value()));
+      break;
+    case kExternalShortArray:
+    case kExternalUnsignedShortArray:
+      __ mov_w(Operand(external_pointer, key, times_2, 0),
+               ToRegister(instr->value()));
+      break;
+    case kExternalIntArray:
+    case kExternalUnsignedIntArray:
+      __ mov(Operand(external_pointer, key, times_4, 0),
+             ToRegister(instr->value()));
+      break;
+    case kExternalFloatArray: {
+      XMMRegister temp(ToDoubleRegister(instr->TempAt(0)));
+      __ cvtsd2ss(temp, ToDoubleRegister(instr->value()));
+      __ movss(Operand(external_pointer, key, times_4, 0), temp);
+      break;
+    }
   }
-  __ mov_b(Operand(external_pointer, key, times_1, 0), eax);
 }