New unified type representation

New unified type representation

Not used yet, only unit tests.

R=jkummerow@chromium.org, svenpanne@chromium.org
BUG=

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

[Sven Panne, 2013-06-05 08:02:41]

LGTM with a few nits.

https://codereview.chromium.org/16154027/diff/9/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16154027/diff/9/src/types.cc#newcode95
src/types.cc:95: default:
Are there really unhandled cases? If yes, how many? Default cases are a road to
maintenance hell...

https://codereview.chromium.org/16154027/diff/9/src/types.cc#newcode246
src/types.cc:246: if (isolate == NULL) isolate =
HeapObject::cast(*type2)->GetIsolate();
The test is a nano-optimization, leaving it out makes things more symmetrical.

https://codereview.chromium.org/16154027/diff/9/src/types.cc#newcode272
src/types.cc:272: if (type->is_bitset())
I would have expected a ternary ?: from you... ;-)

https://codereview.chromium.org/16154027/diff/9/src/types.h
File src/types.h (right):

https://codereview.chromium.org/16154027/diff/9/src/types.h#newcode38
src/types.h:38: class Type : public Object {
Although I hate comments in general, a comment describing the cunning Type
representation might be appropriate here.

https://codereview.chromium.org/16154027/diff/9/src/types.h#newcode117
src/types.h:117: Handle<HeapObject> as_constant() { return
Handle<HeapObject>::cast(handle());}
Cheater! ;-)

https://codereview.chromium.org/16154027/diff/9/src/types.h#newcode122
src/types.h:122: ASSERT(type->IsHeapObject());
No need for this ASSERT, HeapObject::cast already does the same.

[rossberg, 2013-06-05 09:44:24]

PTAL

https://codereview.chromium.org/16154027/diff/9/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16154027/diff/9/src/types.cc#newcode95
src/types.cc:95: default:
On 2013/06/05 08:02:41, Sven Panne wrote:
> Are there really unhandled cases? If yes, how many? Default cases are a road
to
> maintenance hell...

I know, unfortunately, there are several dozens of them -- as many as we have
'internal' heap objects...

If you forget to add a new instance type you'll get an assertion failure right
away, so it's not too bad.

https://codereview.chromium.org/16154027/diff/9/src/types.cc#newcode246
src/types.cc:246: if (isolate == NULL) isolate =
HeapObject::cast(*type2)->GetIsolate();
On 2013/06/05 08:02:41, Sven Panne wrote:
> The test is a nano-optimization, leaving it out makes things more symmetrical.

Done.

https://codereview.chromium.org/16154027/diff/9/src/types.cc#newcode272
src/types.cc:272: if (type->is_bitset())
On 2013/06/05 08:02:41, Sven Panne wrote:
> I would have expected a ternary ?: from you... ;-)

Right you are! :)

https://codereview.chromium.org/16154027/diff/9/src/types.h
File src/types.h (right):

https://codereview.chromium.org/16154027/diff/9/src/types.h#newcode38
src/types.h:38: class Type : public Object {
On 2013/06/05 08:02:41, Sven Panne wrote:
> Although I hate comments in general, a comment describing the cunning Type
> representation might be appropriate here.

Done.

https://codereview.chromium.org/16154027/diff/9/src/types.h#newcode117
src/types.h:117: Handle<HeapObject> as_constant() { return
Handle<HeapObject>::cast(handle());}
On 2013/06/05 08:02:41, Sven Panne wrote:
> Cheater! ;-)

No more, wanted to add an assertion anyway. :)

https://codereview.chromium.org/16154027/diff/9/src/types.h#newcode122
src/types.h:122: ASSERT(type->IsHeapObject());
On 2013/06/05 08:02:41, Sven Panne wrote:
> No need for this ASSERT, HeapObject::cast already does the same.

Yeah, but I wanted to make it very explicit.

[Sven Panne, 2013-06-05 09:48:55]

LGTM still...

[Jakob Kummerow, 2013-06-05 14:53:24]

LGTM. Mostly nits. I guess we can't do much about the handle dereferencing
issue.

https://codereview.chromium.org/16154027/diff/16001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode41
src/types.cc:41: bitset |= union_get(unioned, i)->LubBitset();
nit: {}

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode44
src/types.cc:44: Map* map = this->is_class()? *this->as_class() :
this->as_constant()->map();
nit: space before '?'. If that causes a line break, align ':' with '?'.

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode124
src/types.cc:124: return this->is_class() && *this->as_class() ==
*that->as_class();
I think it would be cleaner to use Handle::is_identical_to() instead of manually
dereferencing and comparing.

Either way, as discussed offline dereferencing handles (implicitly or
explicitly) prevents us from using this during backgrounded compilation. Maybe
add a comment to that effect to make it obvious? (We already have automated
checks, but they're pretty hidden.)

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode261
src/types.cc:261: return *union_get(unioned, 0);
nit: {}

https://codereview.chromium.org/16154027/diff/16001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode51
src/types.h:51: //   Number = Smi \/ Double
As discussed offline, I think we'll need an Integer32 type before we can use
this to replace existing type description systems. But we can always add that
later.

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode133
src/types.h:133: kNull = 1<<0,
nit: any reason not to have spaces around the operator '<<'?

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode171
src/types.h:171: Handle<Type> handle_via(Type* type) {
I find it surprising that |type| is only used to retrieve its isolate. I'd
prefer making this more explicit: either change the signature to take an
Isolate* instead of a Type*, or rename to handle_with_isolate_from(), or
something like that.

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode189
src/types.h:189: int LubBitset();
short comment "Least upper bound"? You can even add that on the same line.

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc
File test/cctest/test-types.cc (right):

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:183: CHECK_NE(AsBitset(*type1), AsBitset(*type2));
nit: {}

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:190: CHECK_NE(AsBitset(*type1), AsBitset(*type2));
nit: {}

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:270: CHECK_NE(0, AsBitset(*type1) & AsBitset(*type2));
nit: {}

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:279: CHECK_EQ(0, AsBitset(*type1) & AsBitset(*type2));
nit: {}

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:354: CHECK_EQ(AsBitset(*type1), AsBitset(*type2));
nit: {} (and again below)

[rossberg, 2013-06-05 15:37:02]

https://codereview.chromium.org/16154027/diff/16001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode41
src/types.cc:41: bitset |= union_get(unioned, i)->LubBitset();
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {}

Done.

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode44
src/types.cc:44: Map* map = this->is_class()? *this->as_class() :
this->as_constant()->map();
On 2013/06/05 14:53:24, Jakob wrote:
> nit: space before '?'. If that causes a line break, align ':' with '?'.

Done.

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode124
src/types.cc:124: return this->is_class() && *this->as_class() ==
*that->as_class();
On 2013/06/05 14:53:24, Jakob wrote:
> I think it would be cleaner to use Handle::is_identical_to() instead of
manually
> dereferencing and comparing.
> 
> Either way, as discussed offline dereferencing handles (implicitly or
> explicitly) prevents us from using this during backgrounded compilation. Maybe
> add a comment to that effect to make it obvious? (We already have automated
> checks, but they're pretty hidden.)

Added a comment to types.h. Left comparisons as is, if that's okay, because I
find them far more readable that way (esp because they are more symmetric).

https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode261
src/types.cc:261: return *union_get(unioned, 0);
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {}

Done.

https://codereview.chromium.org/16154027/diff/16001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode51
src/types.h:51: //   Number = Smi \/ Double
On 2013/06/05 14:53:24, Jakob wrote:
> As discussed offline, I think we'll need an Integer32 type before we can use
> this to replace existing type description systems. But we can always add that
> later.

Yes, let's add stuff once we get there.

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode133
src/types.h:133: kNull = 1<<0,
On 2013/06/05 14:53:24, Jakob wrote:
> nit: any reason not to have spaces around the operator '<<'?

Done.

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode171
src/types.h:171: Handle<Type> handle_via(Type* type) {
On 2013/06/05 14:53:24, Jakob wrote:
> I find it surprising that |type| is only used to retrieve its isolate. I'd
> prefer making this more explicit: either change the signature to take an
> Isolate* instead of a Type*, or rename to handle_with_isolate_from(), or
> something like that.

Done.

https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode189
src/types.h:189: int LubBitset();
On 2013/06/05 14:53:24, Jakob wrote:
> short comment "Least upper bound"? You can even add that on the same line.

Done.

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc
File test/cctest/test-types.cc (right):

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:183: CHECK_NE(AsBitset(*type1), AsBitset(*type2));
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {}

Done.

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:190: CHECK_NE(AsBitset(*type1), AsBitset(*type2));
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {}

Done.

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:270: CHECK_NE(0, AsBitset(*type1) & AsBitset(*type2));
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {}

Done.

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:279: CHECK_EQ(0, AsBitset(*type1) & AsBitset(*type2));
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {}

Done.

https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc...
test/cctest/test-types.cc:354: CHECK_EQ(AsBitset(*type1), AsBitset(*type2));
On 2013/06/05 14:53:24, Jakob wrote:
> nit: {} (and again below)

Done.

[rossberg, 2013-06-05 15:44:03]

Committed patchset #6 manually as r14957 (presubmit successful).