Basic support for tracking smi-only arrays on ia32.

Basic support for tracking smi-only arrays on ia32.

Activated by the flag --smi-only-arrays

Currently not crankshaft support, using flag on non-ia32 platforms will lead to write barrier misses and crashes.

BUG=none
TEST=elements_kind.js

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

[danno, 2011-09-15 14:51:27]

PTAL.

Reviewer guidance:

Initially, most JSArrays, including arrays allocated with "new Array()" in
JavaScript code, have an elements kind of FAST_SMI_ONLY_ELEMENTS (see
bootstrapping.cc). Almost all of the other changes are to track the element_kind
of an object with FastElements, specifically when the first non-smi value is
stored into an array with FAST_SMI_ONLY_ELEMENTS, at which point the element
type changes to FAST_ELEMENTS, or when the first double value is stored in an
SMI-only array, at which point it transitions to FAST_DOUBLE_ELEMENTS. 

New EnsureCanContainXXX routines have been added to objects-inl.h that are used
throughout the runtime code to transition objects when non-Smis are stored in
C++ runtime code. There are additional changes to make sure the map transition
also happens in fast element stores in generated code: The KeyedStore IC needs
to notice this case, but there are built-ins (ArrayPush) that directly
manipulate arrays that also require special code. SetFastElement in objects.cc
notices the special cases that are primarily called from the generic KeyedStore
IC where the transition to FAST_DOUBLE_ELEMENTS happen.

[Sven Panne, 2011-09-16 09:19:32]

http://codereview.chromium.org/7901016/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/bootstrapper.cc#newcode1563
src/bootstrapper.cc:1563: // InternalArrays should not use Smi-Only array
optimizations
Please explain why...

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc#newcode815
src/ia32/ic-ia32.cc:815: // Make sure that the elements are not smi-only
Hmmm, we make sure that the elements *are* smi-only, don't we?

http://codereview.chromium.org/7901016/diff/4001/src/ic.cc
File src/ic.cc (left):

http://codereview.chromium.org/7901016/diff/4001/src/ic.cc#oldcode91
src/ic.cc:91: SmartArrayPointer<char> name =
function->shared()->DebugName()->ToCString();
Debugging remnants? :-)

http://codereview.chromium.org/7901016/diff/4001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects-inl.h#newcode1460
src/objects-inl.h:1460: Heap* heap = GetHeap();
Copy-n-paste from above, perhaps introduce a helper method like AllIsSmiOrHole?

http://codereview.chromium.org/7901016/diff/4001/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/stub-cache.h#newcode728
src/stub-cache.h:728: StoreObjectAction store_objet_action);
Missing 'c', unless we want to switch to french names

http://codereview.chromium.org/7901016/diff/4001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/7901016/diff/4001/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:676: CHECK(array->HasFastElements() ||
array->HasFastSmiOnlyElements());
This disjunction occurs quite frequently, perhaps introduce a separate HasFoo()
predicate for it?

[Rico, 2011-09-16 09:40:10]

first round of comments, did not look at generated code yet

http://codereview.chromium.org/7901016/diff/4001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/heap.cc#newcode2016
src/heap.cc:2016: Map* new_neander_map = Map::cast(obj);
Add comments why we don't wan't the smi optimization here

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4047
src/hydrogen.cc:4047: for (ElementsKind elements_kind = FAST_SMI_ONLY_ELEMENTS;
Use FIRST_ELEMENTS_KIND instead and remove assert? (or am I missing something
subtle here?

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4078
src/hydrogen.cc:4078: if (is_store && (elements_kind == FAST_ELEMENTS ||
is_store && !fast_double_elements ?

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4103
src/hydrogen.cc:4103: ASSERT(elements_kind != FAST_SMI_ONLY_ELEMENTS);
Why can't we end up here with FAST_SMI_ONLY_ELEMENTS ?

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode3271
src/objects.cc:3271: } else if (kind != FAST_SMI_ONLY_ELEMENTS) {
kind == DICTIONARY_ELEMENTS
seems much more readable here

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode3317
src/objects.cc:3317: case FAST_SMI_ONLY_ELEMENTS:
Why not just break here and remove the new extra kind support from
ReferencesObjectsFromElements?

http://codereview.chromium.org/7901016/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode1531
src/objects.h:1531: inline bool HasFastSmiOnlyElements();
Do we want to include something like HasFastTypeElements that return true if we
have any fast element case? Could make some asserts and predicates easier to
read

[Jakob Kummerow, 2011-09-16 16:30:34]

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode243
src/builtins.cc:243: AssertNoAllocation no_gc;
Scope or move this, you're calling EnsureCanContain<whatever> below, which can
do allocations.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode614
src/builtins.cc:614: AssertNoAllocation no_gc;
Scope or move this, you're calling EnsureCanContain<whatever> below.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode631
src/builtins.cc:631: AssertNoAllocation no_gc;
Scope or move this, you're calling EnsureCanContain<whatever> below.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode635
src/builtins.cc:635: if (only_smis && !args[i+1]->IsSmi() &&
nit: spaces around '+'
(also in the next line)

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode744
src/builtins.cc:744: AssertNoAllocation no_gc;
Scope or move this, you're calling EnsureCanContain<whatever> below.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode1000
src/builtins.cc:1000: AssertNoAllocation no_gc;
Scope or move this, you're calling EnsureCanContain<whatever> below.

http://codereview.chromium.org/7901016/diff/4001/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/elements.cc#newcode617
src/elements.cc:617: &element_accessors.fast_elements_handler,
Please add a comment here that the duplicate line is on purpose.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4047
src/hydrogen.cc:4047: for (ElementsKind elements_kind = FAST_SMI_ONLY_ELEMENTS;
On 2011/09/16 09:40:10, Rico wrote:
> Use FIRST_ELEMENTS_KIND instead and remove assert? (or am I missing something
> subtle here?

The point is that fast elements are handled before external elements. You could
express that with a different assert, but some assert is necessary.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4050
src/hydrogen.cc:4050: // After having handled FAST_ELEMENTS and
DICTIONARY_ELEMENTS, we
Update this comment please (either "FAST_SMI_ONLY_ELEMENTS, FAST_ELEMENTS and
DICT...", or just "fast elements and DICT..."

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4105
src/hydrogen.cc:4105: new(zone()) HStoreKeyedFastElement(elements,
Why the line breaks?

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc#newcode821
src/ia32/ic-ia32.cc:821: __ JumpIfNotSmi(eax, &slow);
I think all these added lines (up to and including "__ bind(&not_smi_only);")
must go inside the if-block. Otherwise, if the flag is false, you'll never reach
the code that updates the write barrier.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3867: StoreObjectAction store_objet_action) {
Here's the missing 'c' again

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3898: // Do the store and update the write barrier.
Make sure to preserve
This comment belongs in the 'else' block below.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3900: if (store_objet_action ==
kObjectStoreForcesGeneric) {
And here +='c' too.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3904: ASSERT(store_objet_action ==
kObjectStoreCausesWriteBarrier);
And once more.

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode8333
src/objects.cc:8333: uint32_t index,
Why the new line break?

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode8367
src/objects.cc:8367: // FastDoubleElements
nit: missing period

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode8565
src/objects.cc:8565: return SetFastElement(index,
Why the line breaks?

http://codereview.chromium.org/7901016/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode1709
src/objects.h:1709: // Make sure that elements can contain the specified
elements.
The comments for both these methods deserve better wording. Proposal:
// Makes sure that this object can contain non-Smi objects as elements.

// Makes sure that this object can contain the specified elements.

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode4338
src/objects.h:4338: // storing ElementKind.  for anything other than storing the
ElementKind.
nit: messed up comment (just remove second sentence fragment?)

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js
File test/mjsunit/element-kind.js (right):

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js...
test/mjsunit/element-kind.js:87: assertKind(element_kind.fast_elements, too);
Why fast_elements? Don't all arrays start off with fast_smi_only_elements? Maybe
I'm overlooking something.

[Yang, 2011-09-19 14:00:15]

Still in the process of getting familiar with the code, but got two things I
noticed so far to add.

http://codereview.chromium.org/7901016/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode143
src/objects.h:143: // to efficiently check maps if they have fast elements.
Outdated comment. (Any consequences since the assumption in the comment is now
wrong?)

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js
File test/mjsunit/element-kind.js (right):

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js...
test/mjsunit/element-kind.js:97: assertKind(element_kind.fast_smi_only_elements,
you);
This assert does not pass for me when using the raw patch on top of current
bleeding edge.

[danno, 2011-09-21 14:32:03]

I addressed all the feedback, and made sure that x64 and arm also build and run
with --smi-only-arrays off. Please review again.

http://codereview.chromium.org/7901016/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/bootstrapper.cc#newcode1563
src/bootstrapper.cc:1563: // InternalArrays should not use Smi-Only array
optimizations
On 2011/09/16 09:19:32, Sven wrote:
> Please explain why...

Done.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode243
src/builtins.cc:243: AssertNoAllocation no_gc;
On 2011/09/16 16:30:34, Jakob wrote:
> Scope or move this, you're calling EnsureCanContain<whatever> below, which can
> do allocations.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode614
src/builtins.cc:614: AssertNoAllocation no_gc;
On 2011/09/16 16:30:34, Jakob wrote:
> Scope or move this, you're calling EnsureCanContain<whatever> below.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode631
src/builtins.cc:631: AssertNoAllocation no_gc;
On 2011/09/16 16:30:34, Jakob wrote:
> Scope or move this, you're calling EnsureCanContain<whatever> below.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode635
src/builtins.cc:635: if (only_smis && !args[i+1]->IsSmi() &&
On 2011/09/16 16:30:34, Jakob wrote:
> nit: spaces around '+'
> (also in the next line)

Done.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode744
src/builtins.cc:744: AssertNoAllocation no_gc;
On 2011/09/16 16:30:34, Jakob wrote:
> Scope or move this, you're calling EnsureCanContain<whatever> below.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/builtins.cc#newcode1000
src/builtins.cc:1000: AssertNoAllocation no_gc;
On 2011/09/16 16:30:34, Jakob wrote:
> Scope or move this, you're calling EnsureCanContain<whatever> below.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/elements.cc#newcode617
src/elements.cc:617: &element_accessors.fast_elements_handler,
On 2011/09/16 16:30:34, Jakob wrote:
> Please add a comment here that the duplicate line is on purpose.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/heap.cc#newcode2016
src/heap.cc:2016: Map* new_neander_map = Map::cast(obj);
On 2011/09/16 09:40:10, Rico wrote:
> Add comments why we don't wan't the smi optimization here

Done.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4047
src/hydrogen.cc:4047: for (ElementsKind elements_kind = FAST_SMI_ONLY_ELEMENTS;
On 2011/09/16 09:40:10, Rico wrote:
> Use FIRST_ELEMENTS_KIND instead and remove assert? (or am I missing something
> subtle here?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4050
src/hydrogen.cc:4050: // After having handled FAST_ELEMENTS and
DICTIONARY_ELEMENTS, we
On 2011/09/16 16:30:34, Jakob wrote:
> Update this comment please (either "FAST_SMI_ONLY_ELEMENTS, FAST_ELEMENTS and
> DICT...", or just "fast elements and DICT..."

Done.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4078
src/hydrogen.cc:4078: if (is_store && (elements_kind == FAST_ELEMENTS ||
On 2011/09/16 09:40:10, Rico wrote:
> is_store && !fast_double_elements ?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4103
src/hydrogen.cc:4103: ASSERT(elements_kind != FAST_SMI_ONLY_ELEMENTS);
On 2011/09/16 09:40:10, Rico wrote:
> Why can't we end up here with FAST_SMI_ONLY_ELEMENTS ?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/hydrogen.cc#newcode4105
src/hydrogen.cc:4105: new(zone()) HStoreKeyedFastElement(elements,
On 2011/09/16 16:30:34, Jakob wrote:
> Why the line breaks?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc#newcode815
src/ia32/ic-ia32.cc:815: // Make sure that the elements are not smi-only
On 2011/09/16 09:19:32, Sven wrote:
> Hmmm, we make sure that the elements *are* smi-only, don't we?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/ic-ia32.cc#newcode821
src/ia32/ic-ia32.cc:821: __ JumpIfNotSmi(eax, &slow);
On 2011/09/16 16:30:34, Jakob wrote:
> I think all these added lines (up to and including "__ bind(&not_smi_only);")
> must go inside the if-block. Otherwise, if the flag is false, you'll never
reach
> the code that updates the write barrier.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3867: StoreObjectAction store_objet_action) {
On 2011/09/16 16:30:34, Jakob wrote:
> Here's the missing 'c' again

Done.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3898: // Do the store and update the write barrier.
Make sure to preserve
On 2011/09/16 16:30:34, Jakob wrote:
> This comment belongs in the 'else' block below.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3900: if (store_objet_action ==
kObjectStoreForcesGeneric) {
On 2011/09/16 16:30:34, Jakob wrote:
> And here +='c' too.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3904: ASSERT(store_objet_action ==
kObjectStoreCausesWriteBarrier);
On 2011/09/16 16:30:34, Jakob wrote:
> And once more.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects-inl.h#newcode1460
src/objects-inl.h:1460: Heap* heap = GetHeap();
On 2011/09/16 09:19:32, Sven wrote:
> Copy-n-paste from above, perhaps introduce a helper method like
AllIsSmiOrHole?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode3271
src/objects.cc:3271: } else if (kind != FAST_SMI_ONLY_ELEMENTS) {
On 2011/09/16 09:40:10, Rico wrote:
> kind == DICTIONARY_ELEMENTS
> seems much more readable here

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode3317
src/objects.cc:3317: case FAST_SMI_ONLY_ELEMENTS:
On 2011/09/16 09:40:10, Rico wrote:
> Why not just break here and remove the new extra kind support from
> ReferencesObjectsFromElements?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode8333
src/objects.cc:8333: uint32_t index,
On 2011/09/16 16:30:34, Jakob wrote:
> Why the new line break?

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.cc#newcode8367
src/objects.cc:8367: // FastDoubleElements
On 2011/09/16 16:30:34, Jakob wrote:
> nit: missing period

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode143
src/objects.h:143: // to efficiently check maps if they have fast elements.
On 2011/09/19 14:00:15, Yang wrote:
> Outdated comment. (Any consequences since the assumption in the comment is now
> wrong?)

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode1531
src/objects.h:1531: inline bool HasFastSmiOnlyElements();
On 2011/09/16 09:40:10, Rico wrote:
> Do we want to include something like HasFastTypeElements that return true if
we
> have any fast element case? Could make some asserts and predicates easier to
> read

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode1709
src/objects.h:1709: // Make sure that elements can contain the specified
elements.
On 2011/09/16 16:30:34, Jakob wrote:
> The comments for both these methods deserve better wording. Proposal:
> // Makes sure that this object can contain non-Smi objects as elements.
> 
> // Makes sure that this object can contain the specified elements.

Done.

http://codereview.chromium.org/7901016/diff/4001/src/objects.h#newcode4338
src/objects.h:4338: // storing ElementKind.  for anything other than storing the
ElementKind.
On 2011/09/16 16:30:34, Jakob wrote:
> nit: messed up comment (just remove second sentence fragment?)

Done.

http://codereview.chromium.org/7901016/diff/4001/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/7901016/diff/4001/src/stub-cache.h#newcode728
src/stub-cache.h:728: StoreObjectAction store_objet_action);
On 2011/09/16 09:19:32, Sven wrote:
> Missing 'c', unless we want to switch to french names

Done.

http://codereview.chromium.org/7901016/diff/4001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/7901016/diff/4001/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:676: CHECK(array->HasFastElements() ||
array->HasFastSmiOnlyElements());
On 2011/09/16 09:19:32, Sven wrote:
> This disjunction occurs quite frequently, perhaps introduce a separate
HasFoo()
> predicate for it?

Done.

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js
File test/mjsunit/element-kind.js (right):

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js...
test/mjsunit/element-kind.js:87: assertKind(element_kind.fast_elements, too);
On 2011/09/16 16:30:34, Jakob wrote:
> Why fast_elements? Don't all arrays start off with fast_smi_only_elements?
Maybe
> I'm overlooking something.

Done.

http://codereview.chromium.org/7901016/diff/4001/test/mjsunit/element-kind.js...
test/mjsunit/element-kind.js:97: assertKind(element_kind.fast_smi_only_elements,
you);
I just ran the test on my machine, and it works for me :-)
On 2011/09/19 14:00:15, Yang wrote:
> This assert does not pass for me when using the raw patch on top of current
> bleeding edge.

[Yang, 2011-09-21 14:47:50]

I already had a look at the platform-specific code when porting.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:1490: __ mov(ebx, Operand(esp, 0));  // Copy of
array literal.
Why not load this into edi instead of ebx and remove the same load into edi a
few lines later.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/ic-ia32.cc#newcode737
src/ia32/ic-ia32.cc:737: Label slow, fast, array, extra, not_smi_only;
Only the if-block uses not_smi_only. Put it there please.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/ic-ia32.cc#newcode818
src/ia32/ic-ia32.cc:818: // Non-smis need to call into the runtime if the array
is smi only
Nit: comment should end with a period.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1485: __ bind(&exit);
Remove unused label.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1522: __ bind(&no_fast_elements_check);
Nit: indentation.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3940: __ mov(FieldOperand(edi, ecx, times_2,
FixedArray::kHeaderSize), eax);
Maybe use times_half_pointer_size instead and comment that this is because ecx
is a smi.

[Jakob Kummerow, 2011-09-21 15:42:33]

LGTM.

http://codereview.chromium.org/7901016/diff/28079/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/7901016/diff/28079/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:676: CHECK(array->HasFastElements() ||
array->HasFastSmiOnlyElements());
HasFastTypeElements()

[Sven Panne, 2011-09-22 07:43:25]

LGTM with only a tiny performance consideration (which probably won't make a
difference, anyway... :-P )

http://codereview.chromium.org/7901016/diff/28079/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:390: cmpb(FieldOperand(map,
Map::kBitField2Offset),
Perhaps use a load/sub/cmp instead? I am not really sure if this better, but
this would only involve a single mem access.

[fschneider, 2011-09-22 07:46:38]

stylistic DBCs:

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3907: StoreObjectAction store_object_action) {
I find the name StoreObjectAction a little misleading. Why not just pass the
elements kind instead?
It seems to be used just to decide what to do in case of smi-only  or normal
fast elements.

http://codereview.chromium.org/7901016/diff/28079/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/objects.cc#newcode7589
src/objects.cc:7589: if (elements_kind != FAST_DOUBLE_ELEMENTS) {
Maybe make the condition consistent with the others. Also I think you could
remove the ASSERT below. It really just repeats what the switch-statement
already states, namely we have one of the FAST_... elements.

if (elements_kind == FAST_ELEMENTS ||
    elements_kind == FAST_SMI_ONLY_ELEMENTS) {

[danno, 2011-09-22 11:23:15]

feedback addressed, landing.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:1490: __ mov(ebx, Operand(esp, 0));  // Copy of
array literal.
This was actually another bug. Fixed.
On 2011/09/21 14:47:50, Yang wrote:
> Why not load this into edi instead of ebx and remove the same load into edi a
> few lines later.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/ic-ia32.cc#newcode737
src/ia32/ic-ia32.cc:737: Label slow, fast, array, extra, not_smi_only;
On 2011/09/21 14:47:50, Yang wrote:
> Only the if-block uses not_smi_only. Put it there please.

Done.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/ic-ia32.cc#newcode818
src/ia32/ic-ia32.cc:818: // Non-smis need to call into the runtime if the array
is smi only
On 2011/09/21 14:47:50, Yang wrote:
> Nit: comment should end with a period.

Done.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:390: cmpb(FieldOperand(map,
Map::kBitField2Offset),
The byte load would require a scratch of eax or ebx, and I'd like to avoid
having to have that restriction unless we measure this as a bottleneck.
On 2011/09/22 07:43:25, Sven wrote:
> Perhaps use a load/sub/cmp instead? I am not really sure if this better, but
> this would only involve a single mem access.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1485: __ bind(&exit);
On 2011/09/21 14:47:50, Yang wrote:
> Remove unused label.

Done.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1522: __ bind(&no_fast_elements_check);
On 2011/09/21 14:47:50, Yang wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3907: StoreObjectAction store_object_action) {
On 2011/09/22 07:46:38, fschneider wrote:
> I find the name StoreObjectAction a little misleading. Why not just pass the
> elements kind instead?
> It seems to be used just to decide what to do in case of smi-only  or normal
> fast elements. 

Done.

http://codereview.chromium.org/7901016/diff/28079/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3940: __ mov(FieldOperand(edi, ecx, times_2,
FixedArray::kHeaderSize), eax);
On 2011/09/21 14:47:50, Yang wrote:
> Maybe use times_half_pointer_size instead and comment that this is because ecx
> is a smi.

Done.

http://codereview.chromium.org/7901016/diff/28079/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7901016/diff/28079/src/objects.cc#newcode7589
src/objects.cc:7589: if (elements_kind != FAST_DOUBLE_ELEMENTS) {
On 2011/09/22 07:46:38, fschneider wrote:
> Maybe make the condition consistent with the others. Also I think you could
> remove the ASSERT below. It really just repeats what the switch-statement
> already states, namely we have one of the FAST_... elements.
> 
> if (elements_kind == FAST_ELEMENTS ||
>     elements_kind == FAST_SMI_ONLY_ELEMENTS) {

Done.

http://codereview.chromium.org/7901016/diff/28079/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/7901016/diff/28079/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:676: CHECK(array->HasFastElements() ||
array->HasFastSmiOnlyElements());
On 2011/09/21 15:42:33, Jakob wrote:
> HasFastTypeElements()

Done.

[fschneider, 2011-10-18 10:30:20]

http://codereview.chromium.org/7901016/diff/29001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7901016/diff/29001/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1080: factory->GetElementsTransitionMap(result,
Is this call really necessary? It seems that this always return the same
(current) map because result->map() has already elements kind set to
NON_STRICT_ARGUMENTS_ELEMENTS.