Add support to load/store byte fields.

Add support to load/store byte fields.

This adds a new Byte representation and support for zero-extended
loads in HLoadNamedField and truncated stores in HStoreNamedField.

R=mvstanton@chromium.org

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

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

[Benedikt Meurer, 2013-10-02 10:47:50]

Hey Michael,
Here's the byte load CL we were talking about. PTAL
-- Benedikt

[mvstanton, 2013-10-02 11:13:12]

2 comments, otherwise LGTM.

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-codegen-x64.c...
src/x64/lithium-codegen-x64.cc:2791: MemOperand operand =
MemOperand(ToRegister(instr->object()), offset);
How about a helper function that does the right move based on representation and
sign-extend like:

void LCodeGen::EmitAppropriateMove(Register result, 
    MemOperand operand,
    Representation representation,
    bool sign_extend);

All four of these locations could benefit from that.

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-x64.cc#newcod...
src/x64/lithium-x64.cc:2061:
!instr->access().representation().IsSpecialization() &&
Could you add a comment to summarize this decision?

[Benedikt Meurer, 2013-10-02 11:48:06]

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-codegen-x64.c...
src/x64/lithium-codegen-x64.cc:2791: MemOperand operand =
MemOperand(ToRegister(instr->object()), offset);
On 2013/10/02 11:13:12, mvstanton wrote:
> How about a helper function that does the right move based on representation
and
> sign-extend like:
> 
> void LCodeGen::EmitAppropriateMove(Register result, 
>     MemOperand operand,
>     Representation representation,
>     bool sign_extend);
> 
> All four of these locations could benefit from that.

Done.

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

https://codereview.chromium.org/25696004/diff/1/src/x64/lithium-x64.cc#newcod...
src/x64/lithium-x64.cc:2061:
!instr->access().representation().IsSpecialization() &&
On 2013/10/02 11:13:12, mvstanton wrote:
> Could you add a comment to summarize this decision?

Done.

[mvstanton, 2013-10-02 11:59:06]

right on, looks good.

[Benedikt Meurer, 2013-10-02 13:29:38]

Committed patchset #5 manually as r17079 (presubmit successful).

[Jakob Kummerow, 2013-10-02 16:25:42]

DBC. Unless I'm missing something, this patch is buggy.

https://codereview.chromium.org/25696004/diff/21001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/25696004/diff/21001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4440: __ mov_b(operand,
ToInteger32(operand_value));
Here you're passing an int32_t to a function that expects an int8_t. I'm
surprised this even compiles on Windows.

https://codereview.chromium.org/25696004/diff/21001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4447: __ mov_b(operand, value);
This will fail the CHECK in mov_b when the register allocator happens to pick,
say, edi for |value|. If you had added a test case, you would have noticed ;-)

Same thing twice more below.

[Benedikt Meurer, 2013-10-04 07:13:51]

Committed patchset #5 manually as r17100 (presubmit successful).

[Benedikt Meurer, 2013-10-04 07:14:53]

Fixed and relanded.

https://codereview.chromium.org/25696004/diff/21001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/25696004/diff/21001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4440: __ mov_b(operand,
ToInteger32(operand_value));
On 2013/10/02 16:25:42, Jakob wrote:
> Here you're passing an int32_t to a function that expects an int8_t. I'm
> surprised this even compiles on Windows.

It simply truncates to int8_t according to implicit type conversion rules. Just
what we want here.

https://codereview.chromium.org/25696004/diff/21001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4447: __ mov_b(operand, value);
On 2013/10/02 16:25:42, Jakob wrote:
> This will fail the CHECK in mov_b when the register allocator happens to pick,
> say, edi for |value|. If you had added a test case, you would have noticed ;-)

Good catch, thanks.