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.cc#newcode2866
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.cc#newcode2869
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.cc#newcode2881
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#newcode2428
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#newcode2428
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#newcode137
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#newcode137
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-x64.cc
File src/x64/macro-assembler-x64.cc (right):
https://codereview.chromium.org/61623004/diff/30001/src/x64/macro-assembler-x64.cc#newcode956
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-x64.cc#newcode960
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-x64.cc#newcode974
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-assembler-ia32.cc
File test/cctest/test-macro-assembler-ia32.cc (right):
https://codereview.chromium.org/61623004/diff/30001/test/cctest/test-macro-assembler-ia32.cc#newcode1
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.
https://codereview.chromium.org/61623004/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
