Unify MachineType and RepType.

Unify MachineType and RepType.

MachineType now tracks both the representation and the value type of machine quantities and is used uniformly throughout TurboFan.

These types can now express uint8, int8, uint16, and int16, i.e. signed and unsigned smallish integers. Note that currently only uint8 and uint16 are implemented in the TF backends.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=

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

[Michael Starzinger, 2014-08-13 15:40:05]

LGTM.

https://codereview.chromium.org/470593002/diff/20001/src/compiler/instruction...
File src/compiler/instruction-selector-impl.h (right):

https://codereview.chromium.org/470593002/diff/20001/src/compiler/instruction...
src/compiler/instruction-selector-impl.h:204: if ((location.rep_ & rMask) ==
rFloat64) {
nit: Can we use RepresentationOf here?

https://codereview.chromium.org/470593002/diff/20001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/20001/src/compiler/machine-typ...
src/compiler/machine-type.h:92: // Gets the element size of the machine type.
nit: [...] size in bytes of [...]

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/cod...
File test/cctest/compiler/codegen-tester.h (right):

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/cod...
test/cctest/compiler/codegen-tester.h:210: // return an int value.
nit: s/int/uint/ (twice).

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-machops.cc:14: #define CHECK_UINT32_EQ(x, y) \
*sigh*

[titzer, 2014-08-13 16:12:59]

https://codereview.chromium.org/470593002/diff/20001/src/compiler/instruction...
File src/compiler/instruction-selector-impl.h (right):

https://codereview.chromium.org/470593002/diff/20001/src/compiler/instruction...
src/compiler/instruction-selector-impl.h:204: if ((location.rep_ & rMask) ==
rFloat64) {
On 2014/08/13 15:40:05, Michael Starzinger wrote:
> nit: Can we use RepresentationOf here?

Done.

https://codereview.chromium.org/470593002/diff/20001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/20001/src/compiler/machine-typ...
src/compiler/machine-type.h:92: // Gets the element size of the machine type.
On 2014/08/13 15:40:05, Michael Starzinger wrote:
> nit: [...] size in bytes of [...]

Done.

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/cod...
File test/cctest/compiler/codegen-tester.h (right):

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/cod...
test/cctest/compiler/codegen-tester.h:210: // return an int value.
On 2014/08/13 15:40:05, Michael Starzinger wrote:
> nit: s/int/uint/ (twice).

Done.

[Benedikt Meurer, 2014-08-13 19:42:07]

NOT LGTM because of obvious violation of the style guide.

And why can't we use the type system instead? The MachineType+RepType looks like
a subset of the existing type functionality, unless I'm missing something. So
what's the point of this CL?

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-machops.cc:14: #define CHECK_UINT32_EQ(x, y) \
CheckEqualsHelper for unsigned.

https://codereview.chromium.org/470593002/diff/60001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/60001/src/compiler/machine-typ...
src/compiler/machine-type.h:19: rBit = 1 << 0,
Urghs, WTF? Revival of the hungarian notation? Can we please just follow the
decision to adopt the C++ style guide instead of adding just another custom
naming scheme?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names

[titzer, 2014-08-14 07:50:19]

As discussed in depth with Andreas,

The overall plan is to move to Type* when it is ready, but unfortunately the
representation distinction in that type system is not yet complete, and we are
sloppy everywhere w.r.t to representations.

As in the original CL description, we are actually getting closer to a single
unified type system with this change.

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/470593002/diff/20001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-machops.cc:14: #define CHECK_UINT32_EQ(x, y) \
On 2014/08/13 19:42:07, Benedikt Meurer wrote:
> CheckEqualsHelper for unsigned.

I tried that. C++ doesn't like ambiguous overloads, but it can't resolve an
overload for unsigned integers. O_o

https://codereview.chromium.org/470593002/diff/60001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/60001/src/compiler/machine-typ...
src/compiler/machine-type.h:19: rBit = 1 << 0,
On 2014/08/13 19:42:07, Benedikt Meurer wrote:
> Urghs, WTF? Revival of the hungarian notation? Can we please just follow the
> decision to adopt the C++ style guide instead of adding just another custom
> naming scheme?
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names

I've changed the names to match the recommended k* prefix.

It's important that we keep the distinction between types and representations in
the code to prevent confusion, so these are now:

kRepX  for specific representations
kTypeX for specific types
kMachX for specific type/representation combos

[Benedikt Meurer, 2014-08-14 07:57:02]

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:8: #include "src/ostreams.h"
Please avoid including "src/ostreams.h" in headers. See below.

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:9: #include "src/v8.h"
Same for "src/v8.h". Include what you need instead to avoid further slowing down
compilation.

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:47: inline void PrintMachineTypeUnionTo(OStream& os,
MachineTypeUnion type) {
Please use regular stream syntax:

OStream& operator<<(OStream& os, const MachineTypeUnion&);

and ideally move the implementation to the .cc file and forward declare OStream
in the .h file.

[titzer, 2014-08-14 08:26:47]

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:8: #include "src/ostreams.h"
On 2014/08/14 07:57:02, Benedikt Meurer wrote:
> Please avoid including "src/ostreams.h" in headers. See below.

Done.

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:9: #include "src/v8.h"
On 2014/08/14 07:57:01, Benedikt Meurer wrote:
> Same for "src/v8.h". Include what you need instead to avoid further slowing
down
> compilation.

It needs kPointerSize to define the kMachPtr type.

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:47: inline void PrintMachineTypeUnionTo(OStream& os,
MachineTypeUnion type) {
On 2014/08/14 07:57:02, Benedikt Meurer wrote:
> Please use regular stream syntax:
> 
> OStream& operator<<(OStream& os, const MachineTypeUnion&);
> 
> and ideally move the implementation to the .cc file and forward declare
OStream
> in the .h file.

Done.

[Benedikt Meurer, 2014-08-14 08:28:46]

LGTM if the src/v8.h issue is addressed.

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-type.h
File src/compiler/machine-type.h (right):

https://codereview.chromium.org/470593002/diff/80001/src/compiler/machine-typ...
src/compiler/machine-type.h:9: #include "src/v8.h"
On 2014/08/14 08:26:46, titzer wrote:
> On 2014/08/14 07:57:01, Benedikt Meurer wrote:
> > Same for "src/v8.h". Include what you need instead to avoid further slowing
> down
> > compilation.
> 
> It needs kPointerSize to define the kMachPtr type.

Then include "src/globals.h" which defines it.

[titzer, 2014-08-14 09:20:17]

Committed patchset #8 manually as 23122 (tree was closed).