HSubAllocatedObject instruction allows hydrogen code to carve up

HInnerAllocatedObject instruction allows hydrogen code to carve up 
allocated regions into sub objects. 

BUG=

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

[mvstanton, 2013-03-13 01:13:54]

Hi Michael, hi Hannes,
Here is the HSubAllocatedObject instruction aka "lea." Have a look and let me
know your thoughts, thanks!
--Michael

[Hannes Payer (out of office), 2013-03-13 08:48:39]

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

https://codereview.chromium.org/12812002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:174: V(SubAllocatedObject)                        \
alphabetic order.

https://codereview.chromium.org/12812002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5209: 
Can you move code around that you do not need this forward declaration? It would
be nice to keep ReceiverObjectNeedsWriteBarrier() where it was to have a clean
diff.

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

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:4202: Operand(base, instr->offset()));
fits in one line?

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

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:298: stream->Add("+%d", offset());
do you need a blank before +?

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1093: bool zero_offset = sub_object->offset() == 0;
remove this and just just do return sub_object->offset() == 0 ?
DefineSameAsFirst(result) : DefineAsRegister(result);

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

https://codereview.chromium.org/12812002/diff/1/src/x64/lithium-codegen-x64.c...
src/x64/lithium-codegen-x64.cc:3955: Operand(base, instr->offset()));
fits in one line?

[Michael Starzinger, 2013-03-13 09:12:35]

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

https://codereview.chromium.org/12812002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5325:
DECLARE_CONCRETE_INSTRUCTION(SubAllocatedObject)
Empty newline before the "private:" block.

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

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1093: bool zero_offset = sub_object->offset() == 0;
On 2013/03/13 08:48:39, Hannes Payer wrote:
> remove this and just just do return sub_object->offset() == 0 ?
> DefineSameAsFirst(result) : DefineAsRegister(result);

Does this actually happen in practice? If not I would just remove this
optimization for now. The instruction should be removed completely from the
graph during the canonicalization phase for zero offsets if we ever need this.

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1094: LOperand* base_object =
UseRegister(sub_object->base_object());
The base register and the result register can be the same, better use
"UseRegisterAtStart" for the base register.

[mvstanton, 2013-03-13 10:14:40]

Thanks Hannes, thanks Michael, comments addressed.
All the best,
--Michael

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

https://codereview.chromium.org/12812002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:174: V(SubAllocatedObject)                        \
On 2013/03/13 08:48:39, Hannes Payer wrote:
> alphabetic order.

Done.

https://codereview.chromium.org/12812002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5209: 
On 2013/03/13 08:48:39, Hannes Payer wrote:
> Can you move code around that you do not need this forward declaration? It
would
> be nice to keep ReceiverObjectNeedsWriteBarrier() where it was to have a clean
> diff.

That is much better, thx...I was lazy :).

https://codereview.chromium.org/12812002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5325:
DECLARE_CONCRETE_INSTRUCTION(SubAllocatedObject)
On 2013/03/13 09:12:35, Michael Starzinger wrote:
> Empty newline before the "private:" block.

Done.

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

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:298: stream->Add("+%d", offset());
On 2013/03/13 08:48:39, Hannes Payer wrote:
> do you need a blank before +?
I'll do a blank before and after the plus, that way it'll be like
"sub-allocated-object @ecx = @eax + 16"

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1093: bool zero_offset = sub_object->offset() == 0;
On 2013/03/13 09:12:35, Michael Starzinger wrote:
> On 2013/03/13 08:48:39, Hannes Payer wrote:
> > remove this and just just do return sub_object->offset() == 0 ?
> > DefineSameAsFirst(result) : DefineAsRegister(result);
> 
> Does this actually happen in practice? If not I would just remove this
> optimization for now. The instruction should be removed completely from the
> graph during the canonicalization phase for zero offsets if we ever need this.

good point, thanks. done.

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1093: bool zero_offset = sub_object->offset() == 0;
On 2013/03/13 08:48:39, Hannes Payer wrote:
> remove this and just just do return sub_object->offset() == 0 ?
> DefineSameAsFirst(result) : DefineAsRegister(result);
I got rid of zero_offset, per Michael's advice.

https://codereview.chromium.org/12812002/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1094: LOperand* base_object =
UseRegister(sub_object->base_object());
On 2013/03/13 09:12:35, Michael Starzinger wrote:
> The base register and the result register can be the same, better use
> "UseRegisterAtStart" for the base register.

Done.

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

https://codereview.chromium.org/12812002/diff/1/src/x64/lithium-codegen-x64.c...
src/x64/lithium-codegen-x64.cc:3955: Operand(base, instr->offset()));
On 2013/03/13 08:48:39, Hannes Payer wrote:
> fits in one line?

Done.

[Michael Starzinger, 2013-03-13 10:30:05]

LGTM. I would prefer calling the instruction "HInnerAllocatedObject" instead,
becasue the "Sub" prefix sounds like it is doing a subtraction. But I'll leave
the decision up to you.

[mvstanton, 2013-03-13 11:05:31]

Changed name, thx!