[turbofan] Separate representation type operations from the semantic types.

[turbofan] Make the representation type component independent of the semantic component.

R=rossberg@chromium.org
BUG=

Committed: https://crrev.com/8bd0bd522ed730fd9aa64fae2e08e79a53d42797
Cr-Commit-Position: refs/heads/master@{#26621}

[Jarin, 2015-02-06 12:33:49]

Could you take a look, please?

[Jarin, 2015-02-06 12:39:44]

jarin@chromium.org changed reviewers:
+ rossberg@chromium.org
- rossberg@chrmoium.org

[rossberg, 2015-02-11 12:34:11]

Cool, mostly nits.

https://codereview.chromium.org/904863002/diff/20001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/904863002/diff/20001/src/types.cc#newcode97
src/types.cc:97: DCHECK(this->SemanticIs(Number()));
Why this?

https://codereview.chromium.org/904863002/diff/40001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode164
src/types.cc:164: // Take the representation from the first element.
Nit: add "..., which always is a bitset."

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode167
src/types.cc:167: // Other elements only contribute the semantic part.
Nit: either "contribute to the" or "contribute their"

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode519
src/types.cc:519: return BitsetType::Is(SEMANTIC(this->BitsetLub()),
that->AsBitset());
Don't you need SEMANTIC on 'that' as well?

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode522
src/types.cc:522: return BitsetType::Is(SEMANTIC(this->AsBitset()),
that->BitsetGlb());
Similarly here?

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode684
src/types.cc:684: //    (even when the first element is not a bitset).
This line is obsolete now.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode740
src/types.cc:740: // it should make any decisions based on representations
(i.e.,
"should not make"?

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode995
src/types.cc:995: result->Set(size++, range);
Nit: single line

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

https://codereview.chromium.org/904863002/diff/40001/src/types.h#newcode576
src/types.h:576: bitset GetRepresentation();
Nit: drop the "Get"

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

https://codereview.chromium.org/904863002/diff/40001/test/cctest/test-types.c...
test/cctest/test-types.cc:724: // Rangification: If T->Is(Range(-inf,+inf)) and
!T->Is(None), then
Update comment

https://codereview.chromium.org/904863002/diff/40001/test/cctest/test-types.c...
test/cctest/test-types.cc:1761: CHECK(!T.Intersect(T.ObjectClass,
T.Array)->IsInhabited());
Can we be more precise here? E.g.,

CheckEqual(T.Intersect(T.ObjectClass, T.Array)->Semantic(), T.None);

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

https://codereview.chromium.org/904863002/diff/40001/test/cctest/types-fuzz.h...
test/cctest/types-fuzz.h:230: TypeHandle ProjectRepresentation(TypeHandle t) {
The Type class should probably just have two methods you can forward these to:

  TypeHandle Semantic()
  TypeHandle Representation()

[Jarin, 2015-02-11 16:10:50]

https://codereview.chromium.org/904863002/diff/20001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/904863002/diff/20001/src/types.cc#newcode97
src/types.cc:97: DCHECK(this->SemanticIs(Number()));
On 2015/02/11 12:34:10, rossberg wrote:
> Why this?

I think we should not fail to give a minimum if we have some funny bitset, such
as Signed32 / UntaggedPointer.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode164
src/types.cc:164: // Take the representation from the first element.
On 2015/02/11 12:34:10, rossberg wrote:
> Nit: add "..., which always is a bitset."

Done.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode167
src/types.cc:167: // Other elements only contribute the semantic part.
On 2015/02/11 12:34:10, rossberg wrote:
> Nit: either "contribute to the" or "contribute their"

Done.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode519
src/types.cc:519: return BitsetType::Is(SEMANTIC(this->BitsetLub()),
that->AsBitset());
On 2015/02/11 12:34:10, rossberg wrote:
> Don't you need SEMANTIC on 'that' as well?

I do not think so. That is purely because the representation of the right hand
side cannot possibly affect the result (left hand side representation is empty,
so it is always subtype of the right hand side representation). I agree that
this is not quite obvious, and perhaps a premature optimization.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode522
src/types.cc:522: return BitsetType::Is(SEMANTIC(this->AsBitset()),
that->BitsetGlb());
On 2015/02/11 12:34:10, rossberg wrote:
> Similarly here?

See above.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode684
src/types.cc:684: //    (even when the first element is not a bitset).
On 2015/02/11 12:34:10, rossberg wrote:
> This line is obsolete now.

Done.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode740
src/types.cc:740: // it should make any decisions based on representations
(i.e.,
On 2015/02/11 12:34:10, rossberg wrote:
> "should not make"?

Done.

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode995
src/types.cc:995: result->Set(size++, range);
On 2015/02/11 12:34:10, rossberg wrote:
> Nit: single line

Done.

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

https://codereview.chromium.org/904863002/diff/40001/src/types.h#newcode576
src/types.h:576: bitset GetRepresentation();
On 2015/02/11 12:34:11, rossberg wrote:
> Nit: drop the "Get"

Done.

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

https://codereview.chromium.org/904863002/diff/40001/test/cctest/test-types.c...
test/cctest/test-types.cc:724: // Rangification: If T->Is(Range(-inf,+inf)) and
!T->Is(None), then
On 2015/02/11 12:34:11, rossberg wrote:
> Update comment

Done.

https://codereview.chromium.org/904863002/diff/40001/test/cctest/test-types.c...
test/cctest/test-types.cc:1761: CHECK(!T.Intersect(T.ObjectClass,
T.Array)->IsInhabited());
On 2015/02/11 12:34:11, rossberg wrote:
> Can we be more precise here? E.g.,
> 
> CheckEqual(T.Intersect(T.ObjectClass, T.Array)->Semantic(), T.None);

Done.

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

https://codereview.chromium.org/904863002/diff/40001/test/cctest/types-fuzz.h...
test/cctest/types-fuzz.h:230: TypeHandle ProjectRepresentation(TypeHandle t) {
On 2015/02/11 12:34:11, rossberg wrote:
> The Type class should probably just have two methods you can forward these to:
> 
>   TypeHandle Semantic()
>   TypeHandle Representation()

Done.

[Jarin, 2015-02-12 10:07:35]

jarin@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Jarin, 2015-02-12 10:07:36]

Benedikt, could you take a look at the unit test stuff?

[Benedikt Meurer, 2015-02-12 10:12:04]

https://codereview.chromium.org/904863002/diff/160001/test/unittests/compiler...
File test/unittests/compiler/typer-unittest.cc (right):

https://codereview.chromium.org/904863002/diff/160001/test/unittests/compiler...
test/unittests/compiler/typer-unittest.cc:216: }
Nit:

} //  namespace

[Jarin, 2015-02-12 10:27:25]

Thanks.

https://codereview.chromium.org/904863002/diff/160001/test/unittests/compiler...
File test/unittests/compiler/typer-unittest.cc (right):

https://codereview.chromium.org/904863002/diff/160001/test/unittests/compiler...
test/unittests/compiler/typer-unittest.cc:216: }
On 2015/02/12 10:12:04, Benedikt Meurer wrote:
> Nit:
> 
> } //  namespace

Done.

[rossberg, 2015-02-12 14:39:15]

lgtm

https://codereview.chromium.org/904863002/diff/40001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/904863002/diff/40001/src/types.cc#newcode519
src/types.cc:519: return BitsetType::Is(SEMANTIC(this->BitsetLub()),
that->AsBitset());
On 2015/02/11 16:10:47, jarin wrote:
> On 2015/02/11 12:34:10, rossberg wrote:
> > Don't you need SEMANTIC on 'that' as well?
> 
> I do not think so. That is purely because the representation of the right hand
> side cannot possibly affect the result (left hand side representation is
empty,
> so it is always subtype of the right hand side representation). I agree that
> this is not quite obvious, and perhaps a premature optimization.

Ah, right.

[Jarin, 2015-02-12 15:19:22]

The CQ bit was checked by jarin@chromium.org

[commit-bot: I haz the power, 2015-02-12 15:19:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904863002/180001

[commit-bot: I haz the power, 2015-02-12 15:21:22]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-12 15:21:45]

Patchset 10 (id:??) landed as
https://crrev.com/8bd0bd522ed730fd9aa64fae2e08e79a53d42797
Cr-Commit-Position: refs/heads/master@{#26621}