HStoreNamedField for Smis optimized for x64

HStoreNamedField for Smis optimized for x64

R=verwaest@chromium.org

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

[Toon Verwaest, 2013-12-09 15:51:10]

https://codereview.chromium.org/108413003/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/108413003/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6571: void set_can_omit_smi_tag_store(bool v) {
can_omit_smi_tag_store_ = v; }
I'd rather distinguish between "initializing stores" and stores to existing
fields.

// Indicates whether this is a store to a field that was previously initialized
to a value of the same representation.
bool store_to_initialized_field();

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

https://codereview.chromium.org/108413003/diff/1/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:4011: __ movl(FieldOperand(write_register,
offset), kScratchRegister);
What about just shifting up the value and storing the entire 64 bits if you need
to clear? Seems like that should be at least shorter in number of instruction
bytes.

Especially if the value is constant. In that case you can just load the entire
constant in a register and write that in 1 go. You shouldn't need
kScratchRegisterContainsZero.

[Igor Sheludko, 2013-12-10 09:13:07]

https://codereview.chromium.org/108413003/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/108413003/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6571: void set_can_omit_smi_tag_store(bool v) {
can_omit_smi_tag_store_ = v; }
On 2013/12/09 15:51:10, Toon Verwaest wrote:
> I'd rather distinguish between "initializing stores" and stores to existing
> fields.
> 
> // Indicates whether this is a store to a field that was previously
initialized
> to a value of the same representation.
> bool store_to_initialized_field();

Good suggestion! Done.

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

https://codereview.chromium.org/108413003/diff/1/src/x64/lithium-codegen-x64....
src/x64/lithium-codegen-x64.cc:4011: __ movl(FieldOperand(write_register,
offset), kScratchRegister);
On 2013/12/09 15:51:10, Toon Verwaest wrote:
> What about just shifting up the value and storing the entire 64 bits if you
need
> to clear? Seems like that should be at least shorter in number of instruction
> bytes.
> 
> Especially if the value is constant. In that case you can just load the entire
> constant in a register and write that in 1 go. You shouldn't need
> kScratchRegisterContainsZero.

Done.

[Toon Verwaest, 2013-12-10 09:34:18]

lgtm

[Igor Sheludko, 2013-12-18 11:23:20]

PTAL patch set 3 (cleaner implementation + redundant moves removed).

[Toon Verwaest, 2013-12-18 11:25:50]

A lot cleaner, nice! Would be nice to use an enum for the "initializing store"
flag though.
=

[Igor Sheludko, 2013-12-18 13:33:35]

PTAL, I replaced bool with enum.

[Toon Verwaest, 2013-12-18 14:01:55]

Nice. LGTM

[Igor Sheludko, 2013-12-18 17:53:58]

Committed patchset #4 manually as r18360 (presubmit successful).