Added infrastructure for optimizing new CanvasArray types in WebGL...

Added infrastructure for optimizing new CanvasArray types in WebGL
specification under development. The optimizations are patterned after
those previously done for CanvasPixelArray. This CL adds all of the
necessary framework but continues to use the generic KeyedLoadIC and
KeyedStoreIC code, to create a baseline for benchmarking purposes. The
next CL will add the optimized ICs to ic-ia32.cc and ic-x64.cc.

These new CanvasArray types have different semantics than
CanvasPixelArray; out-of-range values are clamped via C cast
semantics, which is cheaper than the clamping behavior specified by
CanvasPixelArray. Out-of-range indices raise exceptions instead of
being silently ignored.

As part of this work, pulled FloatingPointHelper::AllocateHeapNumber
up to MacroAssembler on ia32 and x64 platforms. Slightly refactored
KeyedLoadIC and KeyedStoreIC. Fixed encoding for fistp_d on x64 and
added a few more instructions that are needed for the new ICs. The
test cases in test-api.cc have been verified by hand to exercise all
of the generated code paths in the forthcoming specialized ICs.


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

[Ken Russell (switch to Gerrit), 2009-10-20 10:45:28]

Please review. Thanks.

[Christian Plesner Hansen, 2009-10-20 11:38:42]

Whew, big one.  Overall lgtm, with comments.

http://codereview.chromium.org/293023/diff/1043/47
File include/v8.h (right):

http://codereview.chromium.org/293023/diff/1043/47#newcode1300
Line 1300: int length);
It's not obvious how the length will be interpreted.  Is it the number of bytes
of data or the number of elements?

http://codereview.chromium.org/293023/diff/1043/72
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/293023/diff/1043/72#newcode620
Line 620: // TODO(kbr): port specialized code.
Unless this will be removed by the next changelist todos should be of the form
TODO(N) where N is an issue tracker entry.  Does this lint?

http://codereview.chromium.org/293023/diff/1043/61
File src/factory.h (right):

http://codereview.chromium.org/293023/diff/1043/61#newcode162
Line 162: static Handle<ExternalArray> NewExternalArray(int length,
This parameters should be on the next line.  Ditto NewPixelArray.

http://codereview.chromium.org/293023/diff/1043/49
File src/heap.cc (right):

http://codereview.chromium.org/293023/diff/1043/49#newcode1677
Line 1677: switch (array_type) {
Couldn't this be implemented using RootIndexForExternalArrayType?

http://codereview.chromium.org/293023/diff/1043/56
File src/ic.cc (right):

http://codereview.chromium.org/293023/diff/1043/56#newcode269
Line 269: switch (elements_kind) {
This dispatch pattern is used a lot.  Maybe use a macro to enumerate the
different types of external arrays like we do for other cases in objects.h and
use it to generate all these dispatches (and possibly map allocation too).

http://codereview.chromium.org/293023/diff/1043/71
File src/objects-inl.h (right):

http://codereview.chromium.org/293023/diff/1043/71#newcode364
Line 364: return (IsExternalByteArray() ||
If all these types come together the enum you can use a range check instead.

http://codereview.chromium.org/293023/diff/1043/71#newcode2017
Line 2017: void ExternalArray::set_external_pointer(void* value,
WriteBarrierMode mode) {
Do you need the 'mode' argument -- it isn't used.

http://codereview.chromium.org/293023/diff/1043/71#newcode2829
Line 2829: if (array->IsExternalArray()) {
A switch on the elements' instance type would be more efficient here.

http://codereview.chromium.org/293023/diff/1043/71#newcode2882
Line 2882: bool JSObject::HasExternalByteElements() {
Consider checking this using the elements' instance type.

http://codereview.chromium.org/293023/diff/1043/51
File src/objects.cc (right):

http://codereview.chromium.org/293023/diff/1043/51#newcode7306
Line 7306: uint32_t index, Object* value) {
Last param should be on next line.

http://codereview.chromium.org/293023/diff/1043/58
File src/runtime.cc (right):

http://codereview.chromium.org/293023/diff/1043/58#newcode2583
Line 2583: // If the target object is a JSObject and has an ExternalArray as
Do we need to deal with heap numbers here too?  If we do we can't assume that an
index fits into an uint32_t for NewIndexError.

http://codereview.chromium.org/293023/diff/1043/58#newcode5319
Line 5319: if (visitor != NULL) {
Can't this check be hoisted out of the loop.  This also applies below.

http://codereview.chromium.org/293023/diff/1043/46
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/293023/diff/1043/46#newcode41
Line 41: #define snprintf _snprintf
You can use SNPrintF from platform.h instead.

[William Hesse, 2009-10-20 12:56:39]

Perhaps you should also check the movw instructions on ia32, to make sure they
are right, and to compare the new x64 versions to.

http://codereview.chromium.org/293023/diff/1043/65
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/293023/diff/1043/65#newcode1258
Line 1258: void Assembler::movw(Register dst, const Operand& src) {
The operand-size prefix 0x66 is necessary.  In this case, the top 48 bits will
be unmodified.  You probably want movzx and movsx.

http://codereview.chromium.org/293023/diff/1043/65#newcode1270
Line 1270: emit(0x89);
One of these two should be 8B.

http://codereview.chromium.org/293023/diff/1043/65#newcode1457
Line 1457: 
The 16-bit version needs the operand-size prefix.  Why isn't the 16-bit version
just movsxlq plus the prefix, based on opcode 0x63?

http://codereview.chromium.org/293023/diff/1043/67
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/293023/diff/1043/67#newcode521
Line 521: void movl(Register dst, Register src);
It wouldn't hurt to comment the block of movl, to say that all moves to a
register are zero-extended to 64-bits.

http://codereview.chromium.org/293023/diff/1043/64
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/293023/diff/1043/64#newcode587
Line 587: return zero;
Why not testq(src, src)
return no_sign;

[Kevin Millikin (Chromium), 2009-10-20 13:35:08]

Some style comments.  I think the approach looks OK.  I'm eager to see the IC
code.

LGTM.

http://codereview.chromium.org/293023/diff/1043/48
File src/api.cc (right):

http://codereview.chromium.org/293023/diff/1043/48#newcode2327
Line 2327: i::Handle<i::ExternalArray> array =
i::Factory::NewExternalArray(length,
The args are kind of scrunched up.  Maybe break right after "=".

http://codereview.chromium.org/293023/diff/1043/74
File src/builtins.h (right):

http://codereview.chromium.org/293023/diff/1043/74#newcode77
Line 77: V(KeyedLoadIC_ExternalByteArray,          KEYED_LOAD_IC, MEGAMORPHIC) \
It's annoying, but you should align the backslashes for the whole macro.

http://codereview.chromium.org/293023/diff/1043/49
File src/heap.cc (right):

http://codereview.chromium.org/293023/diff/1043/49#newcode1693
Line 1693: ASSERT(false && "Illegal external array type");
We usually use UNREACHABLE() here.

http://codereview.chromium.org/293023/diff/1043/55
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/293023/diff/1043/55#newcode406
Line 406: Generate(masm, ExternalReference(Runtime::kSetProperty));
Thank you.

http://codereview.chromium.org/293023/diff/1043/56
File src/ic.cc (right):

http://codereview.chromium.org/293023/diff/1043/56#newcode286
Line 286: ASSERT(false);
We usually use UNREACHABLE() here.

http://codereview.chromium.org/293023/diff/1043/56#newcode311
Line 311: ASSERT(false);
Same.

http://codereview.chromium.org/293023/diff/1043/56#newcode875
Line 875: bool ic_set = false;
This is OK, but you could write:

Handle<Code> stub = Handle<Code>(generic_stub());
if (object->IsJSObject()) {
  ...
  if (receiver->HasExternalArrayElements()) {
    stub = Handle<Code>(external_array_stub(receiver->GetElementsKind()));
  }
}
set_target(*stub);

http://codereview.chromium.org/293023/diff/1043/51
File src/objects.cc (right):

http://codereview.chromium.org/293023/diff/1043/51#newcode2458
Line 2458: case EXTERNAL_FLOAT_ELEMENTS: {
We usually leave off the braces unless needed by a variable declaration in the
case branch.

http://codereview.chromium.org/293023/diff/1043/58
File src/runtime.cc (right):

http://codereview.chromium.org/293023/diff/1043/58#newcode5310
Line 5310: uint32_t len = array->length();
We have Min in utils.h so you can write

uint32_t len = Min(range, array->length());

http://codereview.chromium.org/293023/diff/1043/58#newcode5350
Line 5350: return num_of_elements;
Isn't num_of_elements always len at the end, or am I missing something?

[Ken Russell (switch to Gerrit), 2009-10-20 14:35:13]

http://codereview.chromium.org/293023/diff/1043/47
File include/v8.h (right):

http://codereview.chromium.org/293023/diff/1043/47#newcode1300
Line 1300: int length);
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> It's not obvious how the length will be interpreted.  Is it the number of
bytes
> of data or the number of elements?

Changed to "number_of_elements".

http://codereview.chromium.org/293023/diff/1043/48
File src/api.cc (right):

http://codereview.chromium.org/293023/diff/1043/48#newcode2327
Line 2327: i::Handle<i::ExternalArray> array =
i::Factory::NewExternalArray(length,
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> The args are kind of scrunched up.  Maybe break right after "=".

Done.

http://codereview.chromium.org/293023/diff/1043/72
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/293023/diff/1043/72#newcode620
Line 620: // TODO(kbr): port specialized code.
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Unless this will be removed by the next changelist todos should be of the form
> TODO(N) where N is an issue tracker entry.  Does this lint?

Filed issue 476 and changed TODOs. I think TODO(username) does pass the lint
check, though.

http://codereview.chromium.org/293023/diff/1043/74
File src/builtins.h (right):

http://codereview.chromium.org/293023/diff/1043/74#newcode77
Line 77: V(KeyedLoadIC_ExternalByteArray,          KEYED_LOAD_IC, MEGAMORPHIC) \
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> It's annoying, but you should align the backslashes for the whole macro.

Done.

http://codereview.chromium.org/293023/diff/1043/61
File src/factory.h (right):

http://codereview.chromium.org/293023/diff/1043/61#newcode162
Line 162: static Handle<ExternalArray> NewExternalArray(int length,
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> This parameters should be on the next line.  Ditto NewPixelArray.

Done.

http://codereview.chromium.org/293023/diff/1043/49
File src/heap.cc (right):

http://codereview.chromium.org/293023/diff/1043/49#newcode1677
Line 1677: switch (array_type) {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Couldn't this be implemented using RootIndexForExternalArrayType?

Good point. I didn't realize this since I implemented them in the opposite
order. Done.

http://codereview.chromium.org/293023/diff/1043/49#newcode1693
Line 1693: ASSERT(false && "Illegal external array type");
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> We usually use UNREACHABLE() here.

Fixed here and throughout the code.

http://codereview.chromium.org/293023/diff/1043/56
File src/ic.cc (right):

http://codereview.chromium.org/293023/diff/1043/56#newcode269
Line 269: switch (elements_kind) {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> This dispatch pattern is used a lot.  Maybe use a macro to enumerate the
> different types of external arrays like we do for other cases in objects.h and
> use it to generate all these dispatches (and possibly map allocation too).
> 

The problem is there are a few different kinds of switch statements associated
with these new types. For the time being I'd like to leave them as is, but am
open to refactoring them later.

http://codereview.chromium.org/293023/diff/1043/56#newcode286
Line 286: ASSERT(false);
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> We usually use UNREACHABLE() here.

Done.

http://codereview.chromium.org/293023/diff/1043/56#newcode311
Line 311: ASSERT(false);
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> Same.

Done.

http://codereview.chromium.org/293023/diff/1043/56#newcode875
Line 875: bool ic_set = false;
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> This is OK, but you could write:
> 
> Handle<Code> stub = Handle<Code>(generic_stub());
> if (object->IsJSObject()) {
>   ...
>   if (receiver->HasExternalArrayElements()) {
>     stub = Handle<Code>(external_array_stub(receiver->GetElementsKind()));
>   }
> }
> set_target(*stub);

It turns out this doesn't work because there is no active HandleScope. Based on
your suggestion, I've modified the code here and below to re-assign a Code
pointer.

http://codereview.chromium.org/293023/diff/1043/71
File src/objects-inl.h (right):

http://codereview.chromium.org/293023/diff/1043/71#newcode364
Line 364: return (IsExternalByteArray() ||
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> If all these types come together the enum you can use a range check instead.

OK, made this change and added a comment in objects.h.

http://codereview.chromium.org/293023/diff/1043/71#newcode2017
Line 2017: void ExternalArray::set_external_pointer(void* value,
WriteBarrierMode mode) {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Do you need the 'mode' argument -- it isn't used.

The method signature is defined by the DECL_ACCESSORS macro.

http://codereview.chromium.org/293023/diff/1043/71#newcode2829
Line 2829: if (array->IsExternalArray()) {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> A switch on the elements' instance type would be more efficient here.

Done.

http://codereview.chromium.org/293023/diff/1043/71#newcode2882
Line 2882: bool JSObject::HasExternalByteElements() {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Consider checking this using the elements' instance type.

For parity with the other variants, I'd like to leave this as is.

http://codereview.chromium.org/293023/diff/1043/51
File src/objects.cc (right):

http://codereview.chromium.org/293023/diff/1043/51#newcode2458
Line 2458: case EXTERNAL_FLOAT_ELEMENTS: {
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> We usually leave off the braces unless needed by a variable declaration in the
> case branch.

Done.

http://codereview.chromium.org/293023/diff/1043/51#newcode7306
Line 7306: uint32_t index, Object* value) {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Last param should be on next line.

Done.

http://codereview.chromium.org/293023/diff/1043/58
File src/runtime.cc (right):

http://codereview.chromium.org/293023/diff/1043/58#newcode2583
Line 2583: // If the target object is a JSObject and has an ExternalArray as
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Do we need to deal with heap numbers here too?  If we do we can't assume that
an
> index fits into an uint32_t for NewIndexError.

I don't think so. These arrays are implicitly 4 GB maximum in size, and this
code is only intended to report errors which the WebGL spec requires but which
can't really happen with normal JavaScript arrays.

http://codereview.chromium.org/293023/diff/1043/58#newcode5310
Line 5310: uint32_t len = array->length();
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> We have Min in utils.h so you can write
> 
> uint32_t len = Min(range, array->length());
> 

It turns out to be more gross than this due to a need for a
static_cast<uint32_t>(array->length()), but done.

http://codereview.chromium.org/293023/diff/1043/58#newcode5319
Line 5319: if (visitor != NULL) {
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> Can't this check be hoisted out of the loop.  This also applies below.

Yes, but it was necessary to be careful about returning the number of elements
iterated. Done.

http://codereview.chromium.org/293023/diff/1043/58#newcode5350
Line 5350: return num_of_elements;
On 2009/10/20 13:35:08, Kevin Millikin wrote:
> Isn't num_of_elements always len at the end, or am I missing something?

Yes, it is. Changed along with hoisting out the NULL check on the visitor.

http://codereview.chromium.org/293023/diff/1043/65
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/293023/diff/1043/65#newcode1258
Line 1258: void Assembler::movw(Register dst, const Operand& src) {
On 2009/10/20 12:56:39, William Hesse wrote:
> The operand-size prefix 0x66 is necessary.  In this case, the top 48 bits will
> be unmodified.  You probably want movzx and movsx.

*Thank you* for catching this error. The unit tests should have caught it, but
the nature of the bug was that it would blow away the next array element. I've
expanded the unit tests to contain downward loops, which catch the bug. I also
removed movw(Register, const Operand&) as it was unused and as you point out
it's useless since it doesn't change the high 48 bits.

http://codereview.chromium.org/293023/diff/1043/65#newcode1270
Line 1270: emit(0x89);
On 2009/10/20 12:56:39, William Hesse wrote:
> One of these two should be 8B.

Yes, thank you again. I've removed the variant with opcode 0x8B.

http://codereview.chromium.org/293023/diff/1043/65#newcode1457
Line 1457: 
On 2009/10/20 12:56:39, William Hesse wrote:
> The 16-bit version needs the operand-size prefix.  Why isn't the 16-bit
version
> just movsxlq plus the prefix, based on opcode 0x63?

As we just discussed, these are actually the correct encodings due to the
semantics of these instructions; verified against the 32-bit port.

http://codereview.chromium.org/293023/diff/1043/64
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/293023/diff/1043/64#newcode587
Line 587: return zero;
On 2009/10/20 12:56:39, William Hesse wrote:
> Why not testq(src, src)
> return no_sign;

The high 32 bits of the 64-bit register are not set in this case.

http://codereview.chromium.org/293023/diff/1043/46
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/293023/diff/1043/46#newcode41
Line 41: #define snprintf _snprintf
On 2009/10/20 11:38:42, Christian Plesner Hansen wrote:
> You can use SNPrintF from platform.h instead.

Done.

[Christian Plesner Hansen, 2009-10-20 14:54:09]

http://codereview.chromium.org/293023/diff/1043/58
File src/runtime.cc (right):

http://codereview.chromium.org/293023/diff/1043/58#newcode2583
Line 2583: // If the target object is a JSObject and has an ExternalArray as
What happens if you get -1 represented as a heap number?  There's no guarantee
that integers in smi range will be represented as smis.

[Ken Russell (switch to Gerrit), 2009-10-20 15:08:39]

http://codereview.chromium.org/293023/diff/1043/58
File src/runtime.cc (right):

http://codereview.chromium.org/293023/diff/1043/58#newcode2583
Line 2583: // If the target object is a JSObject and has an ExternalArray as
On 2009/10/20 14:54:10, Christian Plesner Hansen wrote:
> What happens if you get -1 represented as a heap number?  There's no guarantee
> that integers in smi range will be represented as smis.

Probably we will silently drop the operation on the floor rather than throwing
an exception because Array::IndexFromObject will return false, and these objects
don't support adding arbitrary keys. This is what was happening before I added
this explicit check.

[William Hesse, 2009-10-20 15:09:47]

LGTM, with changes.

http://codereview.chromium.org/293023/diff/5013/4029
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/293023/diff/5013/4029#newcode1261
Line 1261: emit_rex_32(src, dst);
Use emit_optional_rex_32.
Ox66 prefix goes before REX.

http://codereview.chromium.org/293023/diff/5013/4029#newcode1492
Line 1492: emit_rex_64(dst, src);
In fact, this one, that you used as an example, is wrong too, I think.  I think
we wrote it before we realized how every register write to a 32-bit register
zeros out the top 32 bits.

http://codereview.chromium.org/293023/diff/5013/4029#newcode1512
Line 1512: emit_rex_64(dst, src);
I don't think this is needed.  Wouldn't the write to the 32-bit register also
zero out the top 32 bits?  I think this should be optional_rex_32, and should be
the exact same code as movzxwl.  In fact, it should just be an inlined call to
movzxwl.

[Ken Russell (switch to Gerrit), 2009-10-20 15:22:28]

http://codereview.chromium.org/293023/diff/5013/4029
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/293023/diff/5013/4029#newcode1261
Line 1261: emit_rex_32(src, dst);
On 2009/10/20 15:09:47, William Hesse wrote:
> Use emit_optional_rex_32.
> Ox66 prefix goes before REX.

Done.

http://codereview.chromium.org/293023/diff/5013/4029#newcode1512
Line 1512: emit_rex_64(dst, src);
On 2009/10/20 15:09:47, William Hesse wrote:
> I don't think this is needed.  Wouldn't the write to the 32-bit register also
> zero out the top 32 bits?  I think this should be optional_rex_32, and should
be
> the exact same code as movzxwl.  In fact, it should just be an inlined call to
> movzxwl.

Not sure. Based on our conversation just now I'm going to leave this as is --
the emission of this instruction is being tested by the code I will send out for
review next and it seems to be working.

[Christian Plesner Hansen, 2009-10-20 16:19:20]

http://codereview.chromium.org/293023/diff/1043/58
File src/runtime.cc (right):

http://codereview.chromium.org/293023/diff/1043/58#newcode2583
Line 2583: // If the target object is a JSObject and has an ExternalArray as
I'm sorry if I'm being slow here but won't that mean that

  a[-1]

will have different behavior from

  var x = -1/3;
  a[x * 3]

[Erik Corry, 2009-10-20 18:32:59]

Worse than that:

x = 0
x = -x
foo[x]   // Won't work, x is -0, not a Smi
x += 1
foo[x]   // Probably won't work either,  x is a non-Smi 1

I checked this with --allow-natives-syntax and the %_IsSmi "function".

[Christian Plesner Hansen, 2009-10-21 04:32:09]

What makes this not work?  As far as I can tell everything but error reporting
on negative indices handle this correctly, and your examples won't trigger that.

[Erik Corry, 2009-10-21 07:56:56]

Good point.  I was just kibitzing without having read the previous conversation
properly.

My point, if I still have one, is that you can get non-Smi integers without
using obviously non-integer operations like division.