Introduce representation types

Introduce representation types

Also:
- improve type pretty-printing,
- update doc comments,
- some renamings for consistency.

R=mstarzinger@chromium.org
BUG=

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

[Michael Starzinger, 2014-03-04 11:59:58]

LGTM with comments.

https://codereview.chromium.org/176843006/diff/1/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/176843006/diff/1/src/types.cc#newcode158
src/types.cc:158: return kTaggedPtr & (
High-level comment: This assumes HeapNumber objects are immutable. This doesn't
hold for double-fields. Better think that through with Toon.

https://codereview.chromium.org/176843006/diff/1/src/types.cc#newcode274
src/types.cc:274: if (value->IsSmi()) return SignedSmall(region);
Shouldn't this be tagged?

https://codereview.chromium.org/176843006/diff/1/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode103
src/types.h:103: // T->Is(SignedSmall())), and the to check whether a specific
case needs
nit: Seems to be a "latter" missing.

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode172
src/types.h:172: V(Allocated,           kReceiver | kFloat | kName)             
      \
Why kFloat here?

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode174
src/types.h:174: V(Detectable,          kDetectableReceiver | kFloat | kName)   
      \
Why kFloat here?

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode175
src/types.h:175: V(NonNumber,           kOddball | kAllocated | kInternal)      
      \
This way kNonNumber contains kFloat, this seems wrong.

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode321
src/types.h:321: enum PrintDim { BOTH_DIMS, SEMANTIC_DIM, REPRESENTATION_DIM };
nit: Can we write out "PrintDimension" at least for the name of the enum type
(not the enum values)?

[rossberg, 2014-03-04 13:53:10]

PTAL

Also got rid of duplication from RAW_REP list macro, and made smi MSB usable for
bitset.

https://codereview.chromium.org/176843006/diff/1/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/176843006/diff/1/src/types.cc#newcode158
src/types.cc:158: return kTaggedPtr & (
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> High-level comment: This assumes HeapNumber objects are immutable. This
doesn't
> hold for double-fields. Better think that through with Toon.

Good point. As discussed off-line, I think we should re-establish that invariant
(not just for the sake of typing) and give those special number cells a
different instance type.

https://codereview.chromium.org/176843006/diff/1/src/types.cc#newcode274
src/types.cc:274: if (value->IsSmi()) return SignedSmall(region);
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> Shouldn't this be tagged?

Good catch.

https://codereview.chromium.org/176843006/diff/1/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode103
src/types.h:103: // T->Is(SignedSmall())), and the to check whether a specific
case needs
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> nit: Seems to be a "latter" missing.

Done.

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode172
src/types.h:172: V(Allocated,           kReceiver | kFloat | kName)             
      \
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> Why kFloat here?

Good question. I removed this thing entirely, since it should now be subsumed by
proper representation types.

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode174
src/types.h:174: V(Detectable,          kDetectableReceiver | kFloat | kName)   
      \
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> Why kFloat here?

Changed to Number.

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode175
src/types.h:175: V(NonNumber,           kOddball | kAllocated | kInternal)      
      \
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> This way kNonNumber contains kFloat, this seems wrong.

Done.

https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode321
src/types.h:321: enum PrintDim { BOTH_DIMS, SEMANTIC_DIM, REPRESENTATION_DIM };
On 2014/03/04 11:59:58, Michael Starzinger wrote:
> nit: Can we write out "PrintDimension" at least for the name of the enum type
> (not the enum values)?

Done.

[Michael Starzinger, 2014-03-04 15:02:24]

LGTM.

https://codereview.chromium.org/176843006/diff/40001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/40001/src/types.h#newcode144
src/types.h:144: V(TaggedPtr,        -1 << 30 | kSemantic)  /* MSB has to be
sign-extended */ \
I would be fine with this. If either Benedikt or Sven want this to be handled
differently, then I defer to alternative suggestions from their end.

[Sven Panne, 2014-03-05 07:22:36]

Not really happy with the confusion of layers...

https://codereview.chromium.org/176843006/diff/40001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/40001/src/types.h#newcode144
src/types.h:144: V(TaggedPtr,        -1 << 30 | kSemantic)  /* MSB has to be
sign-extended */ \
On 2014/03/04 15:02:25, Michael Starzinger wrote:
> I would be fine with this. If either Benedikt or Sven want this to be handled
> differently, then I defer to alternative suggestions from their end.

As already discussed yesterday, I consider this sign-extension here an extremely
ugly and confusing hack. We just want to stuff something which the GC shouldn't
care about into some kind of JavaScript value (a Smi, but that't purely
accidental here). We do similar things e.g. in the exterrnal API, and I don't
see why something similar to DecodeSmiToAligned/EncodeAlignedAsSmi shouldn't
work with the bit set at hand. As it is, the current CL is mixing unrelated
things together.

[Benedikt Meurer, 2014-03-05 07:28:07]

https://codereview.chromium.org/176843006/diff/40001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/40001/src/types.h#newcode144
src/types.h:144: V(TaggedPtr,        -1 << 30 | kSemantic)  /* MSB has to be
sign-extended */ \
On 2014/03/05 07:22:36, Sven Panne wrote:
> On 2014/03/04 15:02:25, Michael Starzinger wrote:
> > I would be fine with this. If either Benedikt or Sven want this to be
handled
> > differently, then I defer to alternative suggestions from their end.
> 
> As already discussed yesterday, I consider this sign-extension here an
extremely
> ugly and confusing hack. We just want to stuff something which the GC
shouldn't
> care about into some kind of JavaScript value (a Smi, but that't purely
> accidental here). We do similar things e.g. in the exterrnal API, and I don't
> see why something similar to DecodeSmiToAligned/EncodeAlignedAsSmi shouldn't
> work with the bit set at hand. As it is, the current CL is mixing unrelated
> things together.

I agree. We should not introduce hacks like this unless there's absolutely no
way to avoid that.

[rossberg, 2014-03-14 10:37:36]

[Somehow forgot to click 'Publish' again a week ago... 8-}]

https://codereview.chromium.org/176843006/diff/40001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/40001/src/types.h#newcode144
src/types.h:144: V(TaggedPtr,        -1 << 30 | kSemantic)  /* MSB has to be
sign-extended */ \
On 2014/03/05 07:22:36, Sven Panne wrote:
> On 2014/03/04 15:02:25, Michael Starzinger wrote:
> > I would be fine with this. If either Benedikt or Sven want this to be
handled
> > differently, then I defer to alternative suggestions from their end.
> 
> As already discussed yesterday, I consider this sign-extension here an
extremely
> ugly and confusing hack. We just want to stuff something which the GC
shouldn't
> care about into some kind of JavaScript value (a Smi, but that't purely
> accidental here). We do similar things e.g. in the exterrnal API, and I don't
> see why something similar to DecodeSmiToAligned/EncodeAlignedAsSmi shouldn't
> work with the bit set at hand. As it is, the current CL is mixing unrelated
> things together.

Michael yesterday (after you had left) expressed very strong disagreement with
your suggestion, and the more I think about the more I agree with him.

There are two ways to implement your suggestion: direct bit fiddling with smi
tags and shift offsets, which is platform dependent and pierces the little
abstraction barrier we have. I hope we can agree that that's a no-go (even
though that's what DecodeSmiToAligned is using, which I consider the real
mistake around here).

Or you extend the Smi class interface to support encoding/decoding unsigneds.
That, however, severely weakens the Smi class, which Michael is strongly opposed
to. In particular, the 'value' method, which currently is the only and always
correct way to extract the value, would cease to have this property. You would
have to start relying on global knowledge to know which projection method to
use, and we have enough of such ambient "invariants" already. So this is also
bad.

The above solution, on the other hand, is entirely local. All it does is solving
a simple isolated problem: how to encode an 31-bit bitfield into an 31-bit
signed integer. This is completely independent of their later injection into
smis and their low-level encoding details (which is all the more relevant since
smis are just one possible backing store for these integers, in zone types it's
a different one). Separation of concerns, pal!

[Michael Starzinger, 2014-03-14 10:49:36]

https://codereview.chromium.org/176843006/diff/40001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/176843006/diff/40001/src/types.h#newcode144
src/types.h:144: V(TaggedPtr,        -1 << 30 | kSemantic)  /* MSB has to be
sign-extended */ \
On 2014/03/14 10:37:37, rossberg wrote:
> On 2014/03/05 07:22:36, Sven Panne wrote:
> > On 2014/03/04 15:02:25, Michael Starzinger wrote:
> > > I would be fine with this. If either Benedikt or Sven want this to be
> handled
> > > differently, then I defer to alternative suggestions from their end.
> > 
> > As already discussed yesterday, I consider this sign-extension here an
> extremely
> > ugly and confusing hack. We just want to stuff something which the GC
> shouldn't
> > care about into some kind of JavaScript value (a Smi, but that't purely
> > accidental here). We do similar things e.g. in the exterrnal API, and I
don't
> > see why something similar to DecodeSmiToAligned/EncodeAlignedAsSmi shouldn't
> > work with the bit set at hand. As it is, the current CL is mixing unrelated
> > things together.
> 
> Michael yesterday (after you had left) expressed very strong disagreement with
> your suggestion, and the more I think about the more I agree with him.
> 
> There are two ways to implement your suggestion: direct bit fiddling with smi
> tags and shift offsets, which is platform dependent and pierces the little
> abstraction barrier we have. I hope we can agree that that's a no-go (even
> though that's what DecodeSmiToAligned is using, which I consider the real
> mistake around here).
> 
> Or you extend the Smi class interface to support encoding/decoding unsigneds.
> That, however, severely weakens the Smi class, which Michael is strongly
opposed
> to. In particular, the 'value' method, which currently is the only and always
> correct way to extract the value, would cease to have this property. You would
> have to start relying on global knowledge to know which projection method to
> use, and we have enough of such ambient "invariants" already. So this is also
> bad.
> 
> The above solution, on the other hand, is entirely local. All it does is
solving
> a simple isolated problem: how to encode an 31-bit bitfield into an 31-bit
> signed integer. This is completely independent of their later injection into
> smis and their low-level encoding details (which is all the more relevant
since
> smis are just one possible backing store for these integers, in zone types
it's
> a different one). Separation of concerns, pal!

So just to clarify: My strong disagreement is against having something like
Smi::FromUsigned() or Smi::unsigned_value() because that weakens a Smi to
basically be either a "Smsi" or "Smui" depending on which methods are used. And
I think that is a bad direction to be heading.

I know that we already use trickery to cast to and from Smi (e.g.
EncodeAlignedAsSmi and DecodeSmiToAligned), but at least those methods are not
part of the "Smi API". Performance forces us to use these hacks, so I won't
start arguing against them. But where I draw the line and will start arguing is
at extending the Smi class in a way that invites everyone to use hacks like
these. Such an API would invite accidental misuse (e.g. creating a Smi with
Smi::FromUnsigned, reading back the value with Smi::value).

[rossberg, 2014-03-18 11:50:29]

Committed patchset #3 manually as r20025 (presubmit successful).