Steps towards unification of number bitset and range types.

Steps towards unification of number bitset and range types.

- New invariant on union types: if the union has a range then the number
  bits in the bitset must be cleared.

- Various tweaks in intersection and union to satisfy the invariant.

- Exposed and used representation bits in range types (and the Limits
  helper class).

- Implemented Glb for ranges so that the Is predicate handles
  ranges correctly.

- Change typer weakening so that it does not rely on GetRange.
  However, the code still seems to be a bit fragile.

- Removed the Smi types from the type system core, instead introduced
  Signed31, Unsigned30 and created constructors for Small(Un)Signed
  that point to the right type for the architecture.

- Punched a hole in the config to be able to get to the isolate so
  that it is possible to allocate heap numbers for newly created
  ranges.

R=rossberg@chromium.org
BUG=

[Jarin, 2014-12-11 18:37:21]

Could you take a look, please?

[rossberg, 2014-12-12 13:57:51]

https://codereview.chromium.org/795713003/diff/80001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:166: } else if
(NodeProperties::GetBounds(value).upper->Is(Type::Signed31())) {
I think this should still refer to SignedSmall; Signed31 maybe shouldn't even be
exposed.

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:873: return Type::Unsigned31();
Similarly here.

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:1252: if (!current_number->IsRange() ||
!previous_number->IsRange()) {
Hm, I wish we could avoid IsRange as well. It's only slightly better here than
GetRange...

https://codereview.chromium.org/795713003/diff/80001/src/types.cc
File src/types.cc (left):

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#oldcode505
src/types.cc:505: if (this->IsBitset() || that->IsBitset()) return true;
I think you want to keep this short-cut to make the bitset case as fast as
possible. But it now has to be && not ||.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode47
src/types.cc:47: // Handle the case of empty operand, so that we do not stretch
So this seems to be a case of violating the point-wise interpretation.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode83
src/types.cc:83: bool TypeImpl<Config>::Contains(typename
TypeImpl<Config>::RangeType* lhs,
Actually, Contains is a misnomer here and above. The Contains below is used for
inhabitance. These functions are checking subtyping, so should be called
RangeType::Is.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode162
src/types.cc:162: if (glb == 0) {
(This seems to be another violation of point-wise.)

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode167
src/types.cc:167: // (The remaining BitsetGlb's are None anyway).
Nit: While we're here, can you move this comment down one line?

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode368
src/types.cc:368: if (min > 0) return glb;
Nit: merge with previous line.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode624
src/types.cc:624: return true;
Nit: any reason for this reformat? It's now inconsistent with 5 lines above.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode636
src/types.cc:636: // 2. If there is a range, then the bitset type does not
contain
Nit: move this down two places (or to the end) for a more natural reading order.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode896
src/types.cc:896: *bits = BitsetType::kNone;
Hm, why do we clear bits here? Can't it still contain unrelated bits?

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode961
src/types.cc:961: result->Set(size++, bits);
Maybe only add bits and range here if non-empty?

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode1197
src/types.cc:1197: SEMANTIC_BITSET_TYPE_LIST(BITSET_CONSTANT)
Nit: indentation

https://codereview.chromium.org/795713003/diff/80001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode200
src/types.h:200: V(Negative31,          1u << 5  | REPRESENTATION(kTagged |
kUntaggedNumber)) \
Hm, I'd really prefer to make any bits referring to odd sizes internal. Would
that work?

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode297
src/types.h:297: //   static i::Isolate* get_isolate(Region* region);
Nit: remove the get_ prefix

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode598
src/types.h:598: static Limits NumberBitsetToLimits(bitset bits, Region*
region);
Nit: ToLimits would seem good enough a name

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode656
src/types.h:656: static bitset NumberBits(bitset bits) {
Move to same place as (Un)signedSmallBits.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode814
src/types.h:814: bitset Representation() { return
REPRESENTATION(this->Get(0)->AsBitset()); }
Please don't expose bitset functionality in this interfaces. Perhaps we should
simply have a protected method Representation in class Type.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode841
src/types.h:841: bitset Representation() { return REPRESENTATION(BitsetLub()); }
Same here.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode846
src/types.h:846: bitset representation, Region* region) {
For the same reason, make representation a Type here.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode851
src/types.h:851: // Make sure the representation does not contain non-number
stuff.
Shouldn't this better be an assertion?

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.cc
File test/cctest/test-types.cc (left):

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.c...
test/cctest/test-types.cc:300: if (SmiValuesAre31Bits()) {
I think we should keep this platform-dependent tests, too, just to be sure and
explicit.

[rossberg, 2014-12-12 15:30:41]

https://codereview.chromium.org/795713003/diff/80001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode814
src/types.h:814: bitset Representation() { return
REPRESENTATION(this->Get(0)->AsBitset()); }
On 2014/12/12 13:57:51, rossberg wrote:
> Please don't expose bitset functionality in this interfaces. Perhaps we should
> simply have a protected method Representation in class Type.

s/in this interfaces/in these public interfaces/

[Jarin, 2015-01-02 16:43:40]

Patchset #6 (id:100001) has been deleted

[Jarin, 2015-01-08 14:30:28]

Thanks. Addressed some of the comments (sorry for the rebase from the last
review). Some work, such as the point-wise representations, should really be
done separately.

https://codereview.chromium.org/795713003/diff/80001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:166: } else if
(NodeProperties::GetBounds(value).upper->Is(Type::Signed31())) {
On 2014/12/12 13:57:50, rossberg wrote:
> I think this should still refer to SignedSmall; Signed31 maybe shouldn't even
be
> exposed.

Interestingly, we test this even on 64-bit architecture, and those tests broke
without changing SignedSmall to Signed31 here.

If you insist, I could change this, but then we would have to disable the test,
too.

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:873: return Type::Unsigned31();
On 2014/12/12 13:57:50, rossberg wrote:
> Similarly here.

This is Unsigned31, so there is no Small* alternative for that.

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:1252: if (!current_number->IsRange() ||
!previous_number->IsRange()) {
On 2014/12/12 13:57:50, rossberg wrote:
> Hm, I wish we could avoid IsRange as well. It's only slightly better here than
> GetRange...

As discussed online, I will try to express this using a range subtyping, but it
should really be a separate change.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc
File src/types.cc (left):

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#oldcode505
src/types.cc:505: if (this->IsBitset() || that->IsBitset()) return true;
On 2014/12/12 13:57:50, rossberg wrote:
> I think you want to keep this short-cut to make the bitset case as fast as
> possible. But it now has to be && not ||.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode47
src/types.cc:47: // Handle the case of empty operand, so that we do not stretch
On 2014/12/12 13:57:50, rossberg wrote:
> So this seems to be a case of violating the point-wise interpretation.

Yes, there is more of these - fixing this does not really belong to this change
list, I think.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode83
src/types.cc:83: bool TypeImpl<Config>::Contains(typename
TypeImpl<Config>::RangeType* lhs,
On 2014/12/12 13:57:50, rossberg wrote:
> Actually, Contains is a misnomer here and above. The Contains below is used
for
> inhabitance. These functions are checking subtyping, so should be called
> RangeType::Is.

Maybe. Are you suggesting that Contains(RangeType*, RangeType*) should be moved
to RangeType::Is(RangeType*) (non-static) and Contains(ConstantType*,
RangeType*) tp ConstantType::Is(RangeType*)?

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode162
src/types.cc:162: if (glb == 0) {
On 2014/12/12 13:57:50, rossberg wrote:
> (This seems to be another violation of point-wise.)

Unfortunately, I had tests failing because there was an empty type that was not
kNone.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode167
src/types.cc:167: // (The remaining BitsetGlb's are None anyway).
On 2014/12/12 13:57:50, rossberg wrote:
> Nit: While we're here, can you move this comment down one line?

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode368
src/types.cc:368: if (min > 0) return glb;
On 2014/12/12 13:57:51, rossberg wrote:
> Nit: merge with previous line.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode624
src/types.cc:624: return true;
On 2014/12/12 13:57:51, rossberg wrote:
> Nit: any reason for this reformat? It's now inconsistent with 5 lines above.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode636
src/types.cc:636: // 2. If there is a range, then the bitset type does not
contain
On 2014/12/12 13:57:50, rossberg wrote:
> Nit: move this down two places (or to the end) for a more natural reading
order.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode896
src/types.cc:896: *bits = BitsetType::kNone;
On 2014/12/12 13:57:51, rossberg wrote:
> Hm, why do we clear bits here? Can't it still contain unrelated bits?

I am not sure what you mean here. The kNone type is not really clearing bits, it
is just canonical form for empty type (which we do require in some places).

As for the "*bits &= ~number_bits" line, we are actually keeping the unrelated
bits, we are not clearing them.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode961
src/types.cc:961: result->Set(size++, bits);
On 2014/12/12 13:57:50, rossberg wrote:
> Maybe only add bits and range here if non-empty?

Let's do that later (if at all), once we figure out what to do with
representations.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode1197
src/types.cc:1197: SEMANTIC_BITSET_TYPE_LIST(BITSET_CONSTANT)
On 2014/12/12 13:57:51, rossberg wrote:
> Nit: indentation

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode200
src/types.h:200: V(Negative31,          1u << 5  | REPRESENTATION(kTagged |
kUntaggedNumber)) \
On 2014/12/12 13:57:51, rossberg wrote:
> Hm, I'd really prefer to make any bits referring to odd sizes internal. Would
> that work?

I am not entirely sure why are these types bad. Eventually, they should be
equivalent to the appropriate ranges.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode297
src/types.h:297: //   static i::Isolate* get_isolate(Region* region);
On 2014/12/12 13:57:51, rossberg wrote:
> Nit: remove the get_ prefix

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode598
src/types.h:598: static Limits NumberBitsetToLimits(bitset bits, Region*
region);
On 2014/12/12 13:57:51, rossberg wrote:
> Nit: ToLimits would seem good enough a name

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode656
src/types.h:656: static bitset NumberBits(bitset bits) {
On 2014/12/12 13:57:51, rossberg wrote:
> Move to same place as (Un)signedSmallBits.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode814
src/types.h:814: bitset Representation() { return
REPRESENTATION(this->Get(0)->AsBitset()); }
On 2014/12/12 13:57:51, rossberg wrote:
> Please don't expose bitset functionality in this interfaces. Perhaps we should
> simply have a protected method Representation in class Type.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode841
src/types.h:841: bitset Representation() { return REPRESENTATION(BitsetLub()); }
On 2014/12/12 13:57:51, rossberg wrote:
> Same here.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode846
src/types.h:846: bitset representation, Region* region) {
On 2014/12/12 13:57:51, rossberg wrote:
> For the same reason, make representation a Type here.

Done.

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode851
src/types.h:851: // Make sure the representation does not contain non-number
stuff.
On 2014/12/12 13:57:51, rossberg wrote:
> Shouldn't this better be an assertion?

Removed this code. (If we want the point-wise representation interpretation, we
should not look there anyway.)

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.cc
File test/cctest/test-types.cc (left):

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.c...
test/cctest/test-types.cc:300: if (SmiValuesAre31Bits()) {
On 2014/12/12 13:57:51, rossberg wrote:
> I think we should keep this platform-dependent tests, too, just to be sure and
> explicit.

Note that all the T.*Small variants do not exist anymore, so we cannot test them
here. I suppose we could test using Type::SignedSmall(), Type::UnsignedSmall(),
but that would be inconsistent with the rest.

[rossberg, 2015-01-15 15:15:12]

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:873: return Type::Unsigned31();
On 2015/01/08 14:30:27, jarin wrote:
> On 2014/12/12 13:57:50, rossberg wrote:
> > Similarly here.
> 
> This is Unsigned31, so there is no Small* alternative for that.

Ah, right. I would prefer the name NonNegative32 for this type.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode83
src/types.cc:83: bool TypeImpl<Config>::Contains(typename
TypeImpl<Config>::RangeType* lhs,
On 2015/01/08 14:30:27, jarin wrote:
> On 2014/12/12 13:57:50, rossberg wrote:
> > Actually, Contains is a misnomer here and above. The Contains below is used
> for
> > inhabitance. These functions are checking subtyping, so should be called
> > RangeType::Is.
> 
> Maybe. Are you suggesting that Contains(RangeType*, RangeType*) should be
moved
> to RangeType::Is(RangeType*) (non-static) and Contains(ConstantType*,
> RangeType*) tp ConstantType::Is(RangeType*)?

Yes, that's perhaps best.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode896
src/types.cc:896: *bits = BitsetType::kNone;
On 2015/01/08 14:30:27, jarin wrote:
> On 2014/12/12 13:57:51, rossberg wrote:
> > Hm, why do we clear bits here? Can't it still contain unrelated bits?
> 
> I am not sure what you mean here. The kNone type is not really clearing bits,
it
> is just canonical form for empty type (which we do require in some places).

Yes, but I'm (still) confused why the empty type is set here if no semantic bits
are left. Can you add a comment? Is this to avoid an uninhabited type?


> As for the "*bits &= ~number_bits" line, we are actually keeping the unrelated
> bits, we are not clearing them.

https://codereview.chromium.org/795713003/diff/80001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode200
src/types.h:200: V(Negative31,          1u << 5  | REPRESENTATION(kTagged |
kUntaggedNumber)) \
On 2015/01/08 14:30:27, jarin wrote:
> On 2014/12/12 13:57:51, rossberg wrote:
> > Hm, I'd really prefer to make any bits referring to odd sizes internal.
Would
> > that work?
> 
> I am not entirely sure why are these types bad. Eventually, they should be
> equivalent to the appropriate ranges.

Mainly to prevent misuse --- for portability, nobody should ever use the odd
width types directly, but always the *Small types. That this CL changes all the
tests to use the odd types sends an even stronger signal in the wrong direction,
IMHO. (I don't understand why the tests needed changing either way.)

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.cc
File test/cctest/test-types.cc (left):

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.c...
test/cctest/test-types.cc:300: if (SmiValuesAre31Bits()) {
On 2015/01/08 14:30:28, jarin wrote:
> On 2014/12/12 13:57:51, rossberg wrote:
> > I think we should keep this platform-dependent tests, too, just to be sure
and
> > explicit.
> 
> Note that all the T.*Small variants do not exist anymore, so we cannot test
them
> here. I suppose we could test using Type::SignedSmall(),
Type::UnsignedSmall(),
> but that would be inconsistent with the rest.

Well, of course you could (should) just add them. ;)

https://codereview.chromium.org/795713003/diff/200001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode160
src/types.cc:160: if (glb == 0) {
Hm, is this special case actually needed?

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode470
src/types.cc:470: typename TypeImpl<Config>::bitset
TypeImpl<Config>::SignedSmallBits() {
Perhaps move these to types-inl.h?

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode826
src/types.cc:826: *lims = Union(Limits(rhs->AsRange()), *lims);
rhs->AsRange seems wrong here. Shouldn't this crash?

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode1137
src/types.cc:1137: BitsetType::New(REPRESENTATION(type->BitsetLub()), region),
region);
You shouldn't need to project the representation here.

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode1213
src/types.cc:1213: // clang-format on
Nit: indentation?

https://codereview.chromium.org/795713003/diff/200001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode91
src/types.h:91: //   Signed31 /\ TaggedInt       (a 'smi')
This doesn't make sense on 64 bit. The original comment still seems correct.

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode112
src/types.h:112: // T->Is(Signed31())), and the latter to check whether a
specific case needs
Similarly here. Please don't encourage use of Signed31.

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode557
src/types.h:557: static bitset SignedSmallBits();
Nit: I think these belong to the Bitset class as BitsetType::SignedSmall etc.

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode681
src/types.h:681: return 7;
Nit: maybe declare this 'inline' but move it to the .cc file, so that it isn't
disconnected from the definition of the array. (Maybe you can even use arraysize
then.)

https://codereview.chromium.org/795713003/diff/200001/test/cctest/compiler/te...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/cctest/compiler/te...
test/cctest/compiler/test-js-typed-lowering.cc:169: static Type* kInt32Types[] =
{Type::Unsigned30(), Type::Negative32(),
Why can't you use the Small types any longer?

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

https://codereview.chromium.org/795713003/diff/200001/test/cctest/test-types....
test/cctest/test-types.cc:917: CheckSub(T.Signed31, T.Number);
Again, I don't see why this should be changed.

https://codereview.chromium.org/795713003/diff/200001/test/cctest/types-fuzz.h
File test/cctest/types-fuzz.h (right):

https://codereview.chromium.org/795713003/diff/200001/test/cctest/types-fuzz....
test/cctest/types-fuzz.h:108: SignedFunction1 = Type::Function(Signed31,
Signed31, region);
Same here.

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
File test/unittests/compiler/change-lowering-unittest.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
test/unittests/compiler/change-lowering-unittest.cc:214:
NodeProperties::SetBounds(val, Bounds(Type::None(), Type::Signed31()));
Isn't it even wrong here? On 64 bits I mean?

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
File test/unittests/compiler/js-builtin-reducer-unittest.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
test/unittests/compiler/js-builtin-reducer-unittest.cc:56: Type::Unsigned30(),  
 Type::Negative32(),  Type::Unsigned31(),
Dito.

[Jarin, 2015-01-16 16:28:40]

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:873: return Type::Unsigned31();
On 2015/01/15 15:15:11, rossberg wrote:
> On 2015/01/08 14:30:27, jarin wrote:
> > On 2014/12/12 13:57:50, rossberg wrote:
> > > Similarly here.
> > 
> > This is Unsigned31, so there is no Small* alternative for that.
> 
> Ah, right. I would prefer the name NonNegative32 for this type.

That's how it was and I changed it because you though Unsigned31 made more
sense. I think Unsigned31 is more consistent.

https://codereview.chromium.org/795713003/diff/80001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.cc#newcode896
src/types.cc:896: *bits = BitsetType::kNone;
On 2015/01/15 15:15:11, rossberg wrote:
> On 2015/01/08 14:30:27, jarin wrote:
> > On 2014/12/12 13:57:51, rossberg wrote:
> > > Hm, why do we clear bits here? Can't it still contain unrelated bits?
> > 
> > I am not sure what you mean here. The kNone type is not really clearing
bits,
> it
> > is just canonical form for empty type (which we do require in some places).
> 
> Yes, but I'm (still) confused why the empty type is set here if no semantic
bits
> are left. Can you add a comment? Is this to avoid an uninhabited type?
> 
> 
> > As for the "*bits &= ~number_bits" line, we are actually keeping the
unrelated
> > bits, we are not clearing them.

This is because of the assertion at types.h:631 'DCHECK(bits == kNone ||
IsInhabited(bits));'. I do not think we should worry about this - as discussed,
we will have to re-design representations anyway.

https://codereview.chromium.org/795713003/diff/80001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/80001/src/types.h#newcode200
src/types.h:200: V(Negative31,          1u << 5  | REPRESENTATION(kTagged |
kUntaggedNumber)) \
On 2015/01/15 15:15:12, rossberg wrote:
> On 2015/01/08 14:30:27, jarin wrote:
> > On 2014/12/12 13:57:51, rossberg wrote:
> > > Hm, I'd really prefer to make any bits referring to odd sizes internal.
> Would
> > > that work?
> > 
> > I am not entirely sure why are these types bad. Eventually, they should be
> > equivalent to the appropriate ranges.
> 
> Mainly to prevent misuse --- for portability, nobody should ever use the odd
> width types directly, but always the *Small types. That this CL changes all
the
> tests to use the odd types sends an even stronger signal in the wrong
direction,
> IMHO. (I don't understand why the tests needed changing either way.)

I changed the tests back (where it made sense). 

I agree that we should eventually move more bitsets to be private, but let's do
it later. I can easily see how the discussion of what should or should not be
private delays the CL by weeks.

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.cc
File test/cctest/test-types.cc (left):

https://codereview.chromium.org/795713003/diff/80001/test/cctest/test-types.c...
test/cctest/test-types.cc:300: if (SmiValuesAre31Bits()) {
On 2015/01/15 15:15:12, rossberg wrote:
> On 2015/01/08 14:30:28, jarin wrote:
> > On 2014/12/12 13:57:51, rossberg wrote:
> > > I think we should keep this platform-dependent tests, too, just to be sure
> and
> > > explicit.
> > 
> > Note that all the T.*Small variants do not exist anymore, so we cannot test
> them
> > here. I suppose we could test using Type::SignedSmall(),
> Type::UnsignedSmall(),
> > but that would be inconsistent with the rest.
> 
> Well, of course you could (should) just add them. ;)

Done.

https://codereview.chromium.org/795713003/diff/200001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode160
src/types.cc:160: if (glb == 0) {
On 2015/01/15 15:15:12, rossberg wrote:
> Hm, is this special case actually needed?

There used to be an assertion in the bitset constructor that insisted on not
having uninhabited types, so I normalize here. We will have to re-do the
representations anyway, so it does not makes sense to spend time on this.

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode470
src/types.cc:470: typename TypeImpl<Config>::bitset
TypeImpl<Config>::SignedSmallBits() {
On 2015/01/15 15:15:12, rossberg wrote:
> Perhaps move these to types-inl.h?

Why? I believe that we should keep the *-inl.h files to absolutely necessary
minimum.

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode826
src/types.cc:826: *lims = Union(Limits(rhs->AsRange()), *lims);
On 2015/01/15 15:15:12, rossberg wrote:
> rhs->AsRange seems wrong here. Shouldn't this crash?

Done.

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode1137
src/types.cc:1137: BitsetType::New(REPRESENTATION(type->BitsetLub()), region),
region);
On 2015/01/15 15:15:12, rossberg wrote:
> You shouldn't need to project the representation here.

Except that at the moment the RangeType insists that the incoming bitset is a
representation.

In any case, this should be re-done anyway, so I'd prefer to keep it.

https://codereview.chromium.org/795713003/diff/200001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode91
src/types.h:91: //   Signed31 /\ TaggedInt       (a 'smi')
On 2015/01/15 15:15:12, rossberg wrote:
> This doesn't make sense on 64 bit. The original comment still seems correct.

Done.

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode112
src/types.h:112: // T->Is(Signed31())), and the latter to check whether a
specific case needs
On 2015/01/15 15:15:12, rossberg wrote:
> Similarly here. Please don't encourage use of Signed31.

Done.

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode557
src/types.h:557: static bitset SignedSmallBits();
On 2015/01/15 15:15:12, rossberg wrote:
> Nit: I think these belong to the Bitset class as BitsetType::SignedSmall etc.

Done.

https://codereview.chromium.org/795713003/diff/200001/src/types.h#newcode681
src/types.h:681: return 7;
On 2015/01/15 15:15:12, rossberg wrote:
> Nit: maybe declare this 'inline' but move it to the .cc file, so that it isn't
> disconnected from the definition of the array. (Maybe you can even use
arraysize
> then.)

Done.

https://codereview.chromium.org/795713003/diff/200001/test/cctest/compiler/te...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/cctest/compiler/te...
test/cctest/compiler/test-js-typed-lowering.cc:169: static Type* kInt32Types[] =
{Type::Unsigned30(), Type::Negative32(),
On 2015/01/15 15:15:12, rossberg wrote:
> Why can't you use the Small types any longer?

Done.

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

https://codereview.chromium.org/795713003/diff/200001/test/cctest/test-types....
test/cctest/test-types.cc:917: CheckSub(T.Signed31, T.Number);
On 2015/01/15 15:15:12, rossberg wrote:
> Again, I don't see why this should be changed.

Because types-fuzz.h does not define SignedSmall. It is added now.

https://codereview.chromium.org/795713003/diff/200001/test/cctest/types-fuzz.h
File test/cctest/types-fuzz.h (right):

https://codereview.chromium.org/795713003/diff/200001/test/cctest/types-fuzz....
test/cctest/types-fuzz.h:108: SignedFunction1 = Type::Function(Signed31,
Signed31, region);
On 2015/01/15 15:15:12, rossberg wrote:
> Same here.

Done.

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
File test/unittests/compiler/change-lowering-unittest.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
test/unittests/compiler/change-lowering-unittest.cc:214:
NodeProperties::SetBounds(val, Bounds(Type::None(), Type::Signed31()));
On 2015/01/15 15:15:12, rossberg wrote:
> Isn't it even wrong here? On 64 bits I mean?

No, this is ok, we are just saying that if incoming value is Signed31 then
tagging is just a simple 32 bit shift by one.

This test does not really make much sense on 64 bits, but we do run it there
anyway.

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
File test/unittests/compiler/js-builtin-reducer-unittest.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
test/unittests/compiler/js-builtin-reducer-unittest.cc:56: Type::Unsigned30(),  
 Type::Negative32(),  Type::Unsigned31(),
On 2015/01/15 15:15:12, rossberg wrote:
> Dito.

Done.

[rossberg, 2015-01-19 11:43:56]

LGTM, only nits.

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/795713003/diff/80001/src/compiler/typer.cc#ne...
src/compiler/typer.cc:873: return Type::Unsigned31();
On 2015/01/16 16:28:39, jarin wrote:
> On 2015/01/15 15:15:11, rossberg wrote:
> > On 2015/01/08 14:30:27, jarin wrote:
> > > On 2014/12/12 13:57:50, rossberg wrote:
> > > > Similarly here.
> > > 
> > > This is Unsigned31, so there is no Small* alternative for that.
> > 
> > Ah, right. I would prefer the name NonNegative32 for this type.
> 
> That's how it was and I changed it because you though Unsigned31 made more
> sense.

Then I was wrong. :)

> I think Unsigned31 is more consistent.

Only internally. Externally it's primarily the counterpart to Negative32.

https://codereview.chromium.org/795713003/diff/200001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/200001/src/types.cc#newcode470
src/types.cc:470: typename TypeImpl<Config>::bitset
TypeImpl<Config>::SignedSmallBits() {
On 2015/01/16 16:28:39, jarin wrote:
> On 2015/01/15 15:15:12, rossberg wrote:
> > Perhaps move these to types-inl.h?
> 
> Why? I believe that we should keep the *-inl.h files to absolutely necessary
> minimum.

Define "necessary". :) These are tiny and will probably be called often. The
other bitset factories are inline as well.

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
File test/unittests/compiler/change-lowering-unittest.cc (right):

https://codereview.chromium.org/795713003/diff/200001/test/unittests/compiler...
test/unittests/compiler/change-lowering-unittest.cc:214:
NodeProperties::SetBounds(val, Bounds(Type::None(), Type::Signed31()));
On 2015/01/16 16:28:40, jarin wrote:
> On 2015/01/15 15:15:12, rossberg wrote:
> > Isn't it even wrong here? On 64 bits I mean?
> 
> No, this is ok, we are just saying that if incoming value is Signed31 then
> tagging is just a simple 32 bit shift by one.
> 
> This test does not really make much sense on 64 bits, but we do run it there
> anyway.

Even if it's ok, I still don't see why it is an improvement. ;)

https://codereview.chromium.org/795713003/diff/220001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/795713003/diff/220001/src/types.cc#newcode1226
src/types.cc:1226: INTERNAL_BITSET_TYPE_LIST(BITSET_CONSTANT)
Nit: one more indentation oddity

https://codereview.chromium.org/795713003/diff/220001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/795713003/diff/220001/src/types.h#newcode340
src/types.h:340: return BitsetType::New(BitsetType::SignedSmallBits());
Nit: I think you can drop the 'Bits" suffix now.

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

https://codereview.chromium.org/795713003/diff/220001/test/cctest/test-types....
test/cctest/test-types.cc:931: CheckSub(T.Signed31, T.Signed32);
Nit: change this back as well. It's actually important that the original is true
on all platforms.

https://codereview.chromium.org/795713003/diff/220001/test/cctest/types-fuzz.h
File test/cctest/types-fuzz.h (right):

https://codereview.chromium.org/795713003/diff/220001/test/cctest/types-fuzz....
test/cctest/types-fuzz.h:49: SmiValuesAre31Bits() ? Type::Signed31(region) :
Type::Signed32(region);
Hm, why don't you call Type::SignedSmall here?