Support external arrays in Crankshaft

Support external arrays in Crankshaft

Add specialized hydrogen and lithium instructions to support loading and storing to external arrays.

Committed: http://code.google.com/p/v8/source/detail?r=7354

[danno, 2011-03-24 10:09:02]

Please review. Thanks!

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

I have a few comments below.

http://codereview.chromium.org/6656001/diff/11001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6656001/diff/11001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:1230: ExternalArrayType array_type() const { return
array_type_; }
The hydrogen instruction has the array type.  You should get rid of the field in
the lithium instruction and delegate in the accessor:

ExternalArrayType array_type() const { return hydrogen()->array_type(); }

http://codereview.chromium.org/6656001/diff/11001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:1657: ExternalArrayType array_type() const { return
array_type_; }
Also delegate to the hydrogen instruction here.

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1177: external_pointer()->PrintNameTo(stream);
It might be useful to print a string representation of the type.  (Same comment
for the store.)

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3082: virtual bool DataEquals(HValue* other) {
return true; }
This function needs to compare the two array types.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2250: __ movsx_b(ToRegister(instr->result()),
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.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2271: Register
result(ToRegister(instr->result()));
Indentation looks off a bit.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2273: __ cmp(Operand(result),
Immediate(0x80000000));
Alternatively (seems more idiomatic to me for some reason):

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

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1852: return (array_type == kExternalUnsignedIntArray)
?
Normally we format this like:

return (array_type == kExternalUnsignedIntArray)
    ? AssignEnvironment(load_instr)
    : load_instr;

Also just above.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1890: ASSERT((representation.IsInteger32() &&
Does this fit on two lines like the one in DoLoadKeyedSpecializedArrayElement?

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1910: temp = FixedTemp(xmm1);
At the moment, xmm0 is never allocated and can be used locally as a dedicated
temp register within lithium instructions.

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

Quick comments. Seems like Kevin is doing a review now as well, so I'll leave
him to it. :)

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2188: Register temp((result.is(eax)) ? ebx :
eax);
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.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2919:
ASSERT(ToRegister(instr->TempAt(0)).is(eax));
Instead of having this assert, let's just do

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

?

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

Addressed feedback, but not everything, so please see my comments. 
Thanks! 
Danno

http://codereview.chromium.org/6656001/diff/11001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6656001/diff/11001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:1230: ExternalArrayType array_type() const { return
array_type_; }
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> The hydrogen instruction has the array type.  You should get rid of the field
in
> the lithium instruction and delegate in the accessor:
> 
> ExternalArrayType array_type() const { return hydrogen()->array_type(); }

Done.

http://codereview.chromium.org/6656001/diff/11001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:1657: ExternalArrayType array_type() const { return
array_type_; }
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> Also delegate to the hydrogen instruction here.

Done.

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1177: external_pointer()->PrintNameTo(stream);
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> It might be useful to print a string representation of the type.  (Same
comment
> for the store.)

Done.

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6656001/diff/11001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3082: virtual bool DataEquals(HValue* other) {
return true; }
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> This function needs to compare the two array types.

good catch. done.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2188: Register temp((result.is(eax)) ? ebx :
eax);
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:
> 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.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2250: __ movsx_b(ToRegister(instr->result()),
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:
> 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.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2271: Register
result(ToRegister(instr->result()));
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:
> Indentation looks off a bit.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2273: __ cmp(Operand(result),
Immediate(0x80000000));
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> Alternatively (seems more idiomatic to me for some reason):
> 
> __ test(result, Immediate(0x80000000));
> DeoptimizeIf(not_zero, instr->environment());

Done.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2919:
ASSERT(ToRegister(instr->TempAt(0)).is(eax));
On 2011/03/24 11:31:20, Mads Ager wrote:
> Instead of having this assert, let's just do
> 
> Register temp = ToRegister(instr->TempAt(0));
> 
> ?

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.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1852: return (array_type == kExternalUnsignedIntArray)
?
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> Normally we format this like:
> 
> return (array_type == kExternalUnsignedIntArray)
>     ? AssignEnvironment(load_instr)
>     : load_instr;
> 
> Also just above.

Done.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1890: ASSERT((representation.IsInteger32() &&
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> Does this fit on two lines like the one in DoLoadKeyedSpecializedArrayElement?

Done.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1910: temp = FixedTemp(xmm1);
On 2011/03/24 11:21:00, Kevin Millikin wrote:
> At the moment, xmm0 is never allocated and can be used locally as a dedicated
> temp register within lithium instructions.

Done.

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

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2188: Register temp((result.is(eax)) ? ebx :
eax);
On 2011/03/24 11:31:20, Mads Ager wrote:
> 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.

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

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

LGTM.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2250: __ movsx_b(ToRegister(instr->result()),
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.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2271: Register
result(ToRegister(instr->result()));
On 2011/03/24 13:18:38, danno wrote:
> 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:
> > Indentation looks off a bit.
> 

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

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

drive-by comment.

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6656001/diff/11001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2273: __ cmp(Operand(result),
Immediate(0x80000000));
Or use:
  __ test(result, Operand(result));
  DeoptimizeIf(negative, instr->environment());
It avoids the 32-bit immediate.