Add signed/unsigned 8-bit and 16-bit Representations

Add signed/unsigned 8-bit and 16-bit Representations to Crankshaft

R=yangguo@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=17588

[danno, 2013-11-06 22:43:10]

PTAL

[Jakob Kummerow, 2013-11-07 09:01:06]

DBC.

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h#ne...
src/property-details.h:137: return kind_ > other.kind_;
This simplification is not correct for e.g. kInteger8 vs. kUInteger8. At least
please add ASSERTs to ensure this function isn't called for representations it
can't handle.

https://codereview.chromium.org/61623004/diff/30001/test/cctest/test-macro-as...
File test/cctest/test-macro-assembler-ia32.cc (right):

https://codereview.chromium.org/61623004/diff/30001/test/cctest/test-macro-as...
test/cctest/test-macro-assembler-ia32.cc:1: // Copyright 2009 the V8 project
authors. All rights reserved.
nit: 2013

[Yang, 2013-11-07 09:32:43]

Added comments.

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2866: return new(zone) Range(-16384, 16383);
Afaik signed integer goes from -2^15 to 2^15-1, which is -32768 to 32767.

I suggest introducing kMaxInt8, kMinInt8, etc. to globals.h and define them
either as -(1<<7) and (1<<7)-1 or -0x80 and 0x7f.

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2869: return new(zone) Range(0, 65535);
Same here. Using a constant would be better.

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2881: return new(zone) Range(0, 255);
While you are at it, this part can also be constantified.

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2428: // Just force the value to be in eax and we're
safe here.
Don't we have the same issue with 16-bit accesses?

How do we actually manage for x64?

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h#ne...
src/property-details.h:137: return kind_ > other.kind_;
On 2013/11/07 09:01:06, Jakob wrote:
> This simplification is not correct for e.g. kInteger8 vs. kUInteger8. At least
> please add ASSERTs to ensure this function isn't called for representations it
> can't handle.

Agree. It's probably better to special-case for representations below
kUInteger16.

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:974: movw(dst, src);
Can we introduce ASSERTs for is_byte_register() for the 8bit and 16bit case?

Also for ia32 of course.

[Benedikt Meurer, 2013-11-07 12:56:04]

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:956: movzxbq(dst, src);
We can safely use movzxbl() here, as all 32-bit instructions auto-zero-extend in
long mode. Depending on src and dst this can avoid the REX prefix.

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:960: movzxwq(dst, src);
Same.

[Benedikt Meurer, 2013-11-07 12:57:55]

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2428: // Just force the value to be in eax and we're
safe here.
On 2013/11/07 09:32:43, Yang wrote:
> Don't we have the same issue with 16-bit accesses?
> 
> How do we actually manage for x64?

In long mode, the REX prefix can be used to access the least significant 8-bit
of edi, esi, ebp and esp.

[danno, 2013-11-07 13:19:56]

Feedback addressed. Please take another look.

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2866: return new(zone) Range(-16384, 16383);
On 2013/11/07 09:32:43, Yang wrote:
> Afaik signed integer goes from -2^15 to 2^15-1, which is -32768 to 32767.
> 
> I suggest introducing kMaxInt8, kMinInt8, etc. to globals.h and define them
> either as -(1<<7) and (1<<7)-1 or -0x80 and 0x7f. 

Done.

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2869: return new(zone) Range(0, 65535);
On 2013/11/07 09:32:43, Yang wrote:
> Same here. Using a constant would be better.

Done.

https://codereview.chromium.org/61623004/diff/30001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2881: return new(zone) Range(0, 255);
On 2013/11/07 09:32:43, Yang wrote:
> While you are at it, this part can also be constantified.

Done.

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2428: // Just force the value to be in eax and we're
safe here.
On 2013/11/07 09:32:43, Yang wrote:
> Don't we have the same issue with 16-bit accesses?
> 
> How do we actually manage for x64?

No, this issue doesn't exist with 16-bit access. In x64, with a REX prefix all
registers can be used in a byte register context.

https://codereview.chromium.org/61623004/diff/30001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2428: // Just force the value to be in eax and we're
safe here.
On 2013/11/07 12:57:55, Benedikt Meurer wrote:
> On 2013/11/07 09:32:43, Yang wrote:
> > Don't we have the same issue with 16-bit accesses?
> > 
> > How do we actually manage for x64?
> 
> In long mode, the REX prefix can be used to access the least significant 8-bit
> of edi, esi, ebp and esp.

Done.

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h#ne...
src/property-details.h:137: return kind_ > other.kind_;
On 2013/11/07 09:32:43, Yang wrote:
> On 2013/11/07 09:01:06, Jakob wrote:
> > This simplification is not correct for e.g. kInteger8 vs. kUInteger8. At
least
> > please add ASSERTs to ensure this function isn't called for representations
it
> > can't handle.
> 
> Agree. It's probably better to special-case for representations below
> kUInteger16.

Added additional logic and a whole bunch of tests.

https://codereview.chromium.org/61623004/diff/30001/src/property-details.h#ne...
src/property-details.h:137: return kind_ > other.kind_;
On 2013/11/07 09:32:43, Yang wrote:
> On 2013/11/07 09:01:06, Jakob wrote:
> > This simplification is not correct for e.g. kInteger8 vs. kUInteger8. At
least
> > please add ASSERTs to ensure this function isn't called for representations
it
> > can't handle.
> 
> Agree. It's probably better to special-case for representations below
> kUInteger16.

Done.

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:956: movzxbq(dst, src);
On 2013/11/07 12:56:05, Benedikt Meurer wrote:
> We can safely use movzxbl() here, as all 32-bit instructions auto-zero-extend
in
> long mode. Depending on src and dst this can avoid the REX prefix.

Done.

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:960: movzxwq(dst, src);
On 2013/11/07 12:56:05, Benedikt Meurer wrote:
> Same.

Done.

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:974: movw(dst, src);
On 2013/11/07 09:32:43, Yang wrote:
> Can we introduce ASSERTs for is_byte_register() for the 8bit and 16bit case?
> 
> Also for ia32 of course.

This isn't an issue on x64, but I added checks on ia32 for byte registers on
byte access.

https://codereview.chromium.org/61623004/diff/30001/test/cctest/test-macro-as...
File test/cctest/test-macro-assembler-ia32.cc (right):

https://codereview.chromium.org/61623004/diff/30001/test/cctest/test-macro-as...
test/cctest/test-macro-assembler-ia32.cc:1: // Copyright 2009 the V8 project
authors. All rights reserved.
On 2013/11/07 09:01:06, Jakob wrote:
> nit: 2013

Done.

[danno, 2013-11-07 13:35:30]

Removed byte register checks.

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:974: movw(dst, src);
On 2013/11/07 09:32:43, Yang wrote:
> Can we introduce ASSERTs for is_byte_register() for the 8bit and 16bit case?
> 
> Also for ia32 of course.

Actually, this isn't necessary here. And in ia32, it is already checked.

[Yang, 2013-11-08 08:45:58]

LGTM once comments are addressed.

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
File src/hydrogen-load-elimination.cc (right):

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
src/hydrogen-load-elimination.cc:277: access.representation().IsUInteger8())
size = 1;
I favor adding brackets for readability.

https://codereview.chromium.org/61623004/diff/210001/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/61623004/diff/210001/src/v8globals.h#newcode586
src/v8globals.h:586: 
I actually had globals.h in mind, not v8globals. We already got kMaxInt etc.
there.

[Sven Panne, 2013-11-08 09:09:30]

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
File src/hydrogen-load-elimination.cc (right):

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
src/hydrogen-load-elimination.cc:277: access.representation().IsUInteger8())
size = 1;
On 2013/11/08 08:45:59, Yang wrote:
> I favor adding brackets for readability.

I would favor something actually OO-like: Adding a method to Representation.
This is plain old imperative C code here...

[danno, 2013-11-08 10:49:11]

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
File src/hydrogen-load-elimination.cc (right):

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
src/hydrogen-load-elimination.cc:277: access.representation().IsUInteger8())
size = 1;
On 2013/11/08 08:45:59, Yang wrote:
> I favor adding brackets for readability.

Done.

https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
src/hydrogen-load-elimination.cc:277: access.representation().IsUInteger8())
size = 1;
On 2013/11/08 09:09:30, Sven Panne wrote:
> On 2013/11/08 08:45:59, Yang wrote:
> > I favor adding brackets for readability.
> 
> I would favor something actually OO-like: Adding a method to Representation.
> This is plain old imperative C code here...

Done.

https://codereview.chromium.org/61623004/diff/210001/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/61623004/diff/210001/src/v8globals.h#newcode586
src/v8globals.h:586: 
On 2013/11/08 08:45:59, Yang wrote:
> I actually had globals.h in mind, not v8globals. We already got kMaxInt etc.
> there.

Done.

[Yang, 2013-11-08 10:52:26]

On 2013/11/08 10:49:11, danno wrote:
>
https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
> File src/hydrogen-load-elimination.cc (right):
> 
>
https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
> src/hydrogen-load-elimination.cc:277: access.representation().IsUInteger8())
> size = 1;
> On 2013/11/08 08:45:59, Yang wrote:
> > I favor adding brackets for readability.
> 
> Done.
> 
>
https://codereview.chromium.org/61623004/diff/210001/src/hydrogen-load-elimin...
> src/hydrogen-load-elimination.cc:277: access.representation().IsUInteger8())
> size = 1;
> On 2013/11/08 09:09:30, Sven Panne wrote:
> > On 2013/11/08 08:45:59, Yang wrote:
> > > I favor adding brackets for readability.
> > 
> > I would favor something actually OO-like: Adding a method to Representation.
> > This is plain old imperative C code here...
> 
> Done.
> 
> https://codereview.chromium.org/61623004/diff/210001/src/v8globals.h
> File src/v8globals.h (right):
> 
>
https://codereview.chromium.org/61623004/diff/210001/src/v8globals.h#newcode586
> src/v8globals.h:586: 
> On 2013/11/08 08:45:59, Yang wrote:
> > I actually had globals.h in mind, not v8globals. We already got kMaxInt etc.
> > there.
> 
> Done.

lgtm.

[danno, 2013-11-08 11:50:36]

Committed patchset #6 manually as r17588 (presubmit successful).