Reorganize object type enum, such that proxies are no longer in the middle

Reorganize object type enum, such that proxies are no longer in the middle
of the range of proper JS objects.

Unfortunately, callable types no longer form a range now. However, there
are only two anyway. We put them at either end of the range of JS object
types so that certain compares can be combined.

R=erik.corry@gmail.com,kmillikin@chromium.org
BUG=
TEST=

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

[Kevin Millikin (Chromium), 2011-09-07 10:13:12]

Couple of small suggestions.

http://codereview.chromium.org/7737036/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7737036/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1922: ASSERT(LAST_SPEC_OBJECT_TYPE == LAST_TYPE);
This could be a static assert?

http://codereview.chromium.org/7737036/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1934: __ cmp(temp2,
Operand(FIRST_SPEC_OBJECT_TYPE));
Aren't the flags still set from the previous compare?  You could do something
like:

__ CompareObjectType(...)
if (..."Function"...) {
  __ b(lt, is_false);
  __ b(eq, is_true);
} else {
  __ b(le, is_false);
}
?

http://codereview.chromium.org/7737036/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7737036/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1749: ASSERT(LAST_SPEC_OBJECT_TYPE ==
LAST_TYPE);
STATIC_ASSERT?

http://codereview.chromium.org/7737036/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1761: __ CmpInstanceType(temp,
FIRST_SPEC_OBJECT_TYPE);
Same comment about restructuring this code a bit as on the ARM platform.

[rossberg, 2011-09-07 14:57:31]

http://codereview.chromium.org/7737036/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7737036/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1922: ASSERT(LAST_SPEC_OBJECT_TYPE == LAST_TYPE);
On 2011/09/07 10:13:12, Kevin Millikin wrote:
> This could be a static assert?

Done.

http://codereview.chromium.org/7737036/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1934: __ cmp(temp2,
Operand(FIRST_SPEC_OBJECT_TYPE));
On 2011/09/07 10:13:12, Kevin Millikin wrote:
> Aren't the flags still set from the previous compare?  You could do something
> like:
> 
> __ CompareObjectType(...)
> if (..."Function"...) {
>   __ b(lt, is_false);
>   __ b(eq, is_true);
> } else {
>   __ b(le, is_false);
> }
> ?

Of course. That was the whole intent of the ordering I chose, and I overlooked
to take advantage of it here. 8-}

http://codereview.chromium.org/7737036/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7737036/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1749: ASSERT(LAST_SPEC_OBJECT_TYPE ==
LAST_TYPE);
On 2011/09/07 10:13:12, Kevin Millikin wrote:
> STATIC_ASSERT?

Done.

http://codereview.chromium.org/7737036/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1761: __ CmpInstanceType(temp,
FIRST_SPEC_OBJECT_TYPE);
On 2011/09/07 10:13:12, Kevin Millikin wrote:
> Same comment about restructuring this code a bit as on the ARM platform.

Done. (And also on x64.)

[Erik Corry, 2011-09-15 09:13:46]

LGTM

http://codereview.chromium.org/7737036/diff/3002/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7737036/diff/3002/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1749: // either "Object" or "Function", only
the branch conditions differ.
The test for Object involves two comparisons and two conditional jumps, but I
think the range is contiguous.  That means it could be done with a subtract, a
compare and an unsigned conditional branch.

http://codereview.chromium.org/7737036/diff/3002/src/macros.py
File src/macros.py (right):

http://codereview.chromium.org/7737036/diff/3002/src/macros.py#newcode119
src/macros.py:119: # This is the same as being either a function or an object in
V8 terminology.
Is this comment accurate and up to date?  I think it covers JSFunction, JSObject
and the new proxy types now, right?

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

http://codereview.chromium.org/7737036/diff/3002/src/objects-inl.h#newcode527
src/objects-inl.h:527: (HeapObject::cast(this)->map()->instance_type() >=
FIRST_JS_PROXY_TYPE &&
This would be neater if you extract the instance type into a variable first.

[Kevin Millikin (Chromium), 2011-09-15 10:07:17]

Also LGTM.

[rossberg, 2011-09-15 14:06:06]

PTAL, I implemented the more efficient test you suggested on all architectures.

I did rebase my branch to address one of your comments, so unfortunately, all
the relative diffs are useless now. Sorry. But the new changes for the
optimization are only in lithium-codegen-* and in lithium-x64.*, so you can
limit your 2nd look to those. :)

http://codereview.chromium.org/7737036/diff/3002/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7737036/diff/3002/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1749: // either "Object" or "Function", only
the branch conditions differ.
On 2011/09/15 09:13:46, Erik Corry wrote:
> The test for Object involves two comparisons and two conditional jumps, but I
> think the range is contiguous.  That means it could be done with a subtract, a
> compare and an unsigned conditional branch.

Done, here and in other backends.

http://codereview.chromium.org/7737036/diff/3002/src/macros.py
File src/macros.py (right):

http://codereview.chromium.org/7737036/diff/3002/src/macros.py#newcode119
src/macros.py:119: # This is the same as being either a function or an object in
V8 terminology.
On 2011/09/15 09:13:46, Erik Corry wrote:
> Is this comment accurate and up to date?  I think it covers JSFunction,
JSObject
> and the new proxy types now, right?

No, a proper version is on HEAD already (from a different branch). I rebased to
make this change disappear.

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

http://codereview.chromium.org/7737036/diff/3002/src/objects-inl.h#newcode527
src/objects-inl.h:527: (HeapObject::cast(this)->map()->instance_type() >=
FIRST_JS_PROXY_TYPE &&
On 2011/09/15 09:13:46, Erik Corry wrote:
> This would be neater if you extract the instance type into a variable first.

Done.