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-lowering.cc
File src/compiler/change-lowering.cc (right):
https://codereview.chromium.org/795713003/diff/80001/src/compiler/change-lowering.cc#newcode166
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#newcode873
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#newcode1252
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.cc#oldcode300
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.
https://codereview.chromium.org/795713003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
