Use a special EnumLength field to indicate number of valid enum cache values.

Use a special EnumLength field to indicate number of valid enum cache values.

This is preparatory work for sharing Enum Caches.

Committed: https://code.google.com/p/v8/source/detail?r=12400

[Toon Verwaest, 2012-07-30 09:35:40]

PTAL.

This CL adds a field EnumLength to the map that indicates the number of valid
enum elements for that map. It changes the generated code to use this field
rather than the length of the cached array. 

With the change, we also always create an enum cache and store it, even if
cache_result was indicated to be false. Instead, we just do not set the
enum_length field (we keep it as kInvalidEnumCache). Firstly this reduces the
obvious computation to create the enum properties in case the cache "misses".
More importantly, by only relying on the EnumLength field, the generated code
for checking if an enum cache is valid gets simplified.

Since we now just rely on kInvalidEnumCache to flag a map to not be "simple
enum", we do not need the empty descriptor array for that anymore. So we can
just set the EnumLength to 0 in case of an empty descriptor array, and load the
empty_fixed_array as its cache. (Or skip the loop altogether, as I do in the
full codegen).

[Michael Starzinger, 2012-08-06 15:06:22]

First round of comments.

To improve the generated code for getting the EnumLength value, we should
consider moving that to the LSB of bit_field3 and then just masking away the
other bits. Thereby we get a perfectly SMI tagged value and safe a lot of
instructions and complexity. It seems we only use the tagged representation of
that value in generated code.

https://chromiumcodereview.appspot.com/10824079/diff/21/src/handles.cc
File src/handles.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/21/src/handles.cc#newcod...
src/handles.cc:726: // Count encountering the empty descriptor array as a cache
hit.
Hmm, "encountering" is an adjective, I would just drop the comment completely.

https://chromiumcodereview.appspot.com/10824079/diff/21/src/handles.cc#newcod...
src/handles.cc:729: return
Handle<FixedArray>(isolate->factory()->empty_fixed_array());
No need for a copy constructor of the handle here!

https://chromiumcodereview.appspot.com/10824079/diff/21/src/handles.cc#newcod...
src/handles.cc:742: storage = isolate->factory()->NewFixedArray(num_enum);
Can we just merge the handle declarations and the allocations into one line?

https://chromiumcodereview.appspot.com/10824079/diff/21/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/21/src/heap.cc#newcode2078
src/heap.cc:2078: Map::EnumLengthBits::update(
This is a little convoluted. Better use ...

map->set_bit_field3(Map::LastAddedBits::encode(Map::kNoneAdded) | 
                    Map::EnumLengthBits::encode(Map::kInvalidEnumCache));

https://chromiumcodereview.appspot.com/10824079/diff/21/src/hydrogen-instruct...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10824079/diff/21/src/hydrogen-instruct...
src/hydrogen-instructions.h:109: V(MapEnumLength)                             \
Alpha-sort!

https://chromiumcodereview.appspot.com/10824079/diff/21/src/ia32/full-codegen...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/21/src/ia32/full-codegen...
src/ia32/full-codegen-ia32.cc:1093: Label no_descriptors;
Move this up above the __ bind() but below the comment. Also drop all the empty
newlines around the label. The label isn't the central thing here, the assembler
instructions are.

https://chromiumcodereview.appspot.com/10824079/diff/21/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://chromiumcodereview.appspot.com/10824079/diff/21/src/ia32/lithium-ia32...
src/ia32/lithium-ia32.h:93: V(MapEnumLength)                              \
Alpha-sort!

https://chromiumcodereview.appspot.com/10824079/diff/21/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10824079/diff/21/src/objects.h#newcode...
src/objects.h:4888: int LastAdded() {
Can we move LastAdded() up just above SetLastAdded()?

[Toon Verwaest, 2012-08-07 10:49:47]

Addressed comments, PTAL.

http://codereview.chromium.org/10824079/diff/21/src/handles.cc
File src/handles.cc (right):

http://codereview.chromium.org/10824079/diff/21/src/handles.cc#newcode726
src/handles.cc:726: // Count encountering the empty descriptor array as a cache
hit.
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> Hmm, "encountering" is an adjective, I would just drop the comment completely.

Done.

http://codereview.chromium.org/10824079/diff/21/src/handles.cc#newcode729
src/handles.cc:729: return
Handle<FixedArray>(isolate->factory()->empty_fixed_array());
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> No need for a copy constructor of the handle here!

Done.

http://codereview.chromium.org/10824079/diff/21/src/handles.cc#newcode742
src/handles.cc:742: storage = isolate->factory()->NewFixedArray(num_enum);
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> Can we just merge the handle declarations and the allocations into one line?

Done.

http://codereview.chromium.org/10824079/diff/21/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/10824079/diff/21/src/heap.cc#newcode2078
src/heap.cc:2078: Map::EnumLengthBits::update(
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> This is a little convoluted. Better use ...
> 
> map->set_bit_field3(Map::LastAddedBits::encode(Map::kNoneAdded) | 
>                     Map::EnumLengthBits::encode(Map::kInvalidEnumCache));

Done.

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

http://codereview.chromium.org/10824079/diff/21/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:109: V(MapEnumLength)                             \
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> Alpha-sort!

Done.

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

http://codereview.chromium.org/10824079/diff/21/src/ia32/full-codegen-ia32.cc...
src/ia32/full-codegen-ia32.cc:1093: Label no_descriptors;
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> Move this up above the __ bind() but below the comment. Also drop all the
empty
> newlines around the label. The label isn't the central thing here, the
assembler
> instructions are.

Done.

http://codereview.chromium.org/10824079/diff/21/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/10824079/diff/21/src/ia32/lithium-ia32.h#newco...
src/ia32/lithium-ia32.h:93: V(MapEnumLength)                              \
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> Alpha-sort!

Done.

http://codereview.chromium.org/10824079/diff/21/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/10824079/diff/21/src/objects.h#newcode4888
src/objects.h:4888: int LastAdded() {
On 2012/08/06 15:06:22, Michael Starzinger wrote:
> Can we move LastAdded() up just above SetLastAdded()?

Done.

[Jakob Kummerow, 2012-08-28 09:50:42]

The diff from patch set 1 to 2 LGTM.

[Jakob Kummerow, 2012-08-28 12:24:44]

ARM port LGTM if you address the comments.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/full-codeg...
File src/arm/full-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/full-codeg...
src/arm/full-codegen-arm.cc:1151: __ Push(r1, r2, r0);
Please try to keep the registers that are being pushed in descending order.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/lithium-ar...
src/arm/lithium-arm.h:1013: "map-enum-length")
nit: fits on one line

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/lithium-ar...
src/arm/lithium-arm.h:1014: DECLARE_HYDROGEN_ACCESSOR(MapEnumLength)
I don't see why you need this (not on ia32 either).

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
File src/arm/macro-assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:3748: cmp(r3,
Operand(Smi::FromInt(Map::kInvalidEnumCache)));
On ia32, this compares against Smi::FromInt(0). Is the ARM version different on
purpose?

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:3753: // Check that there are no elements.
Register rcx contains the current JS
s/rcx/r2/, I guess

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:3756: cmp(r2, r6);
s/r6/empty_fixed_array_value/ please

[Toon Verwaest, 2012-08-28 12:30:23]

Addressed comments.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/full-codeg...
File src/arm/full-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/full-codeg...
src/arm/full-codegen-arm.cc:1151: __ Push(r1, r2, r0);
On 2012/08/28 12:24:44, Jakob wrote:
> Please try to keep the registers that are being pushed in descending order.

Done.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/lithium-ar...
src/arm/lithium-arm.h:1013: "map-enum-length")
On 2012/08/28 12:24:44, Jakob wrote:
> nit: fits on one line

Done.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/lithium-ar...
src/arm/lithium-arm.h:1014: DECLARE_HYDROGEN_ACCESSOR(MapEnumLength)
On 2012/08/28 12:24:44, Jakob wrote:
> I don't see why you need this (not on ia32 either).

Done.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
File src/arm/macro-assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:3748: cmp(r3,
Operand(Smi::FromInt(Map::kInvalidEnumCache)));
On 2012/08/28 12:24:44, Jakob wrote:
> On ia32, this compares against Smi::FromInt(0). Is the ARM version different
on
> purpose?

Done.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:3753: // Check that there are no elements.
Register rcx contains the current JS
On 2012/08/28 12:24:44, Jakob wrote:
> s/rcx/r2/, I guess

Done.

https://chromiumcodereview.appspot.com/10824079/diff/17008/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:3756: cmp(r2, r6);
On 2012/08/28 12:24:44, Jakob wrote:
> s/r6/empty_fixed_array_value/ please

Done.