Allow direct allocation in old pointer space.

Allow direct allocation in old pointer space.

BUG=

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

[mvstanton, 2013-03-05 09:35:57]

Hi Hannes, see my comments re: a unit test and a minor code comment. This is
great stuff!
--Michael

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc#newcode1064
src/hydrogen.cc:1064: }
This is a cool example of how you request in hydrogen an old space allocation.
This place is allocating a FixedArray, I think as part of a JSArray? I think it
might be good to also have the "parent" JSArray object be in the old space too.

Also, there is no intelligence behind the pretenuring, yet. Is this just to show
that if you turn on the flag the mechanism works?

In that case maybe build a little extra stuff. What about
1) A runtime function to ask:

SPACE GetObjectSpace(obj)

2) a unit test that uses this to show that the old space allocations work. You'd
turn on the new flag for that test, otherwise it'll continue being off by
default.

--> Soon enough there'll be all kinds of neat rules behind the pre-tenuring
decision, and the complexity of this simple unit test will grow nicely.

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

https://codereview.chromium.org/12314155/diff/12001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5519: MacroAssembler::OLD_POINTER_SPACE);
Maybe it's nice to have a variable for the space to allocate in, setting it via
ternary operator like
Space space = instr->hydrogen()->CanAllocateInOldPointerSpace()
    ? MacroAssembler::OLD_POINTER_SPACE
    : MacroAssembler::NEW_SPACE;

then you just need one __ Allocate() call.

[danno, 2013-03-05 12:18:17]

https://codereview.chromium.org/12314155/diff/12001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5654: __ AllocateInNewSpace(size, result,
scratch, scratch2, deferred->entry(),
Seems like the above three __ Allocates can be combined into a single one, where
you only change how the size is calculated (or not) and which space is selected.

https://codereview.chromium.org/12314155/diff/12001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/12314155/diff/12001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.h:672: OLD_POINTER_SPACE
In general, is there any way to combine AllocationTarget with AllocationFlags?
Might be cleaner?

https://codereview.chromium.org/12314155/diff/12001/src/factory.cc
File src/factory.cc (left):

https://codereview.chromium.org/12314155/diff/12001/src/factory.cc#oldcode950
src/factory.cc:950: 
nit: not sure you should make a whitespace change part of this CL.

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc#newcode1063
src/hydrogen.cc:1063: flags | HAllocate::CAN_ALLOCATE_IN_OLD_POINTER_SPACE);
I think this belongs inside HAllocate for now if the intent is to test old space
allocation, that way we catch all new allocations and test them as they get
added.

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

https://codereview.chromium.org/12314155/diff/12001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5519: MacroAssembler::OLD_POINTER_SPACE);
+1
On 2013/03/05 09:35:57, mvstanton wrote:
> Maybe it's nice to have a variable for the space to allocate in, setting it
via
> ternary operator like
> Space space = instr->hydrogen()->CanAllocateInOldPointerSpace()
>     ? MacroAssembler::OLD_POINTER_SPACE
>     : MacroAssembler::NEW_SPACE;
> 
> then you just need one __ Allocate() call.

https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1420: // always safe because the limit of the
heap is always aligned.
Is the limit of the heap also still aligned in the old space case?

https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1460: }
nit: Can you move this up to where AllocationInNewSpace used to be? Makes the
diff easier.

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

https://codereview.chromium.org/12314155/diff/12001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:3691: ExternalReference
MacroAssembler::GetTopAddress(AllocationTarget target) {
Do these belong in the macro assembler at all? Elsewhere we put commonly used
ExternalReference generators in assembler.cc so that they can be shared among
platforms.

[Michael Starzinger, 2013-03-07 19:19:38]

https://codereview.chromium.org/12314155/diff/23001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/12314155/diff/23001/src/assembler.cc#newcode1166
src/assembler.cc:1166: 
Two empty newlines between functions. Also one occurrence above and one below.

https://codereview.chromium.org/12314155/diff/23001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/23001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:170: AddInstruction(new(zone) HAllocate(context(),
Either keep the old line-breaks or also move the context() onto the next line.

https://codereview.chromium.org/12314155/diff/23001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/12314155/diff/23001/src/flag-definitions.h#ne...
src/flag-definitions.h:166: DEFINE_bool(pretenure_objects, false, "pretenure
objects")
This flag is very ambiguous, how about calling it pretenure_literals? Or at
least make the description string more explanatory.

https://codereview.chromium.org/12314155/diff/23001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/23001/src/hydrogen.cc#newcode1061
src/hydrogen.cc:1061: if (FLAG_pretenure_objects) {
This is incorrect for fast-double elements kind as it would allocate the
FixedDoubleArray backing store in old-pointer-space instead of old-data-space.

[Hannes Payer (out of office), 2013-03-11 17:16:31]

https://codereview.chromium.org/12314155/diff/12001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5654: __ AllocateInNewSpace(size, result,
scratch, scratch2, deferred->entry(),
On 2013/03/05 12:18:18, danno wrote:
> Seems like the above three __ Allocates can be combined into a single one,
where
> you only change how the size is calculated (or not) and which space is sel

I was able to combine the two Allocate calls if size is a constant operand.

https://codereview.chromium.org/12314155/diff/12001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/12314155/diff/12001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.h:672: OLD_POINTER_SPACE
On 2013/03/05 12:18:18, danno wrote:
> In general, is there any way to combine AllocationTarget with AllocationFlags?
> Might be cleaner?

Done.

https://codereview.chromium.org/12314155/diff/12001/src/factory.cc
File src/factory.cc (left):

https://codereview.chromium.org/12314155/diff/12001/src/factory.cc#oldcode950
src/factory.cc:950: 
On 2013/03/05 12:18:18, danno wrote:
> nit: not sure you should make a whitespace change part of this CL.

I had some changes in that file, which went away over time. I put the whitespace
back.

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc#newcode1063
src/hydrogen.cc:1063: flags | HAllocate::CAN_ALLOCATE_IN_OLD_POINTER_SPACE);
On 2013/03/05 12:18:18, danno wrote:
> I think this belongs inside HAllocate for now if the intent is to test old
space
> allocation, that way we catch all new allocations and test them as they get
> added.

I think it is cleaner to add it to the callers of HAllocate.

https://codereview.chromium.org/12314155/diff/12001/src/hydrogen.cc#newcode1064
src/hydrogen.cc:1064: }
On 2013/03/05 09:35:57, mvstanton wrote:
> This is a cool example of how you request in hydrogen an old space allocation.
> This place is allocating a FixedArray, I think as part of a JSArray? I think
it
> might be good to also have the "parent" JSArray object be in the old space
too.
> 
> Also, there is no intelligence behind the pretenuring, yet. Is this just to
show
> that if you turn on the flag the mechanism works?
> 
> In that case maybe build a little extra stuff. What about
> 1) A runtime function to ask:
> 
> SPACE GetObjectSpace(obj)
> 
> 2) a unit test that uses this to show that the old space allocations work.
You'd
> turn on the new flag for that test, otherwise it'll continue being off by
> default.
> 
> --> Soon enough there'll be all kinds of neat rules behind the pre-tenuring
> decision, and the complexity of this simple unit test will grow nicely.

Done.

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

https://codereview.chromium.org/12314155/diff/12001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5519: MacroAssembler::OLD_POINTER_SPACE);
On 2013/03/05 12:18:18, danno wrote:
> +1
> On 2013/03/05 09:35:57, mvstanton wrote:
> > Maybe it's nice to have a variable for the space to allocate in, setting it
> via
> > ternary operator like
> > Space space = instr->hydrogen()->CanAllocateInOldPointerSpace()
> >     ? MacroAssembler::OLD_POINTER_SPACE
> >     : MacroAssembler::NEW_SPACE;
> > 
> > then you just need one __ Allocate() call.
> 

after re-factoring not needed anymore.

https://codereview.chromium.org/12314155/diff/12001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5519: MacroAssembler::OLD_POINTER_SPACE);
On 2013/03/05 09:35:57, mvstanton wrote:
> Maybe it's nice to have a variable for the space to allocate in, setting it
via
> ternary operator like
> Space space = instr->hydrogen()->CanAllocateInOldPointerSpace()
>     ? MacroAssembler::OLD_POINTER_SPACE
>     : MacroAssembler::NEW_SPACE;
> 
> then you just need one __ Allocate() call.

Done.

https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1420: // always safe because the limit of the
heap is always aligned.
On 2013/03/05 12:18:18, danno wrote:
> Is the limit of the heap also still aligned in the old space case?

Done.

https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1460: }
On 2013/03/05 12:18:18, danno wrote:
> nit: Can you move this up to where AllocationInNewSpace used to be? Makes the
> diff easier. 

Done.

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

https://codereview.chromium.org/12314155/diff/12001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:3691: ExternalReference
MacroAssembler::GetTopAddress(AllocationTarget target) {
On 2013/03/05 12:18:18, danno wrote:
> Do these belong in the macro assembler at all? Elsewhere we put commonly used
> ExternalReference generators in assembler.cc so that they can be shared among
> platforms.

Done. I removed these helper functions.

https://codereview.chromium.org/12314155/diff/23001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/12314155/diff/23001/src/assembler.cc#newcode1166
src/assembler.cc:1166: 
On 2013/03/07 19:19:38, Michael Starzinger wrote:
> Two empty newlines between functions. Also one occurrence above and one below.

Done.

https://codereview.chromium.org/12314155/diff/23001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/23001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:170: AddInstruction(new(zone) HAllocate(context(),
On 2013/03/07 19:19:38, Michael Starzinger wrote:
> Either keep the old line-breaks or also move the context() onto the next line.

Done.

https://codereview.chromium.org/12314155/diff/23001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/12314155/diff/23001/src/flag-definitions.h#ne...
src/flag-definitions.h:166: DEFINE_bool(pretenure_objects, false, "pretenure
objects")
On 2013/03/07 19:19:38, Michael Starzinger wrote:
> This flag is very ambiguous, how about calling it pretenure_literals? Or at
> least make the description string more explanatory.

Done.

https://codereview.chromium.org/12314155/diff/23001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12314155/diff/23001/src/hydrogen.cc#newcode1061
src/hydrogen.cc:1061: if (FLAG_pretenure_objects) {
On 2013/03/07 19:19:38, Michael Starzinger wrote:
> This is incorrect for fast-double elements kind as it would allocate the
> FixedDoubleArray backing store in old-pointer-space instead of old-data-space.

Done.

[danno, 2013-03-12 11:28:20]

https://codereview.chromium.org/12314155/diff/30036/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/12314155/diff/30036/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1217: ExternalReference allocation_top =
((flags & PRETENURE) != 0) ?
Does it make sense to create two utility functions:

GetAllocationTopReference() 
GetAllocationLimitReference() 

That take flags and return ExternalReferences? You use this in multiple places,
and it will become even more useful when you add old data space support.

https://codereview.chromium.org/12314155/diff/30036/src/macro-assembler.h
File src/macro-assembler.h (right):

https://codereview.chromium.org/12314155/diff/30036/src/macro-assembler.h#new...
src/macro-assembler.h:54: PRETENURE = 1 << 4
Do you think it makes sense to call this PRETENURE_OLD_POINTER_SPACE?

[Hannes Payer (out of office), 2013-03-12 12:37:27]

https://codereview.chromium.org/12314155/diff/30036/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/12314155/diff/30036/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1217: ExternalReference allocation_top =
((flags & PRETENURE) != 0) ?
On 2013/03/12 11:28:20, danno wrote:
> Does it make sense to create two utility functions:
> 
> GetAllocationTopReference() 
> GetAllocationLimitReference() 
> 
> That take flags and return ExternalReferences? You use this in multiple
places,
> and it will become even more useful when you add old data space support.

Done.

https://codereview.chromium.org/12314155/diff/30036/src/macro-assembler.h
File src/macro-assembler.h (right):

https://codereview.chromium.org/12314155/diff/30036/src/macro-assembler.h#new...
src/macro-assembler.h:54: PRETENURE = 1 << 4
On 2013/03/12 11:28:20, danno wrote:
> Do you think it makes sense to call this PRETENURE_OLD_POINTER_SPACE?
> 

Done.

[Hannes Payer (out of office), 2013-03-13 07:50:07]

On 2013/03/12 12:37:27, Hannes Payer wrote:
>
https://codereview.chromium.org/12314155/diff/30036/src/ia32/macro-assembler-...
> File src/ia32/macro-assembler-ia32.cc (right):
> 
>
https://codereview.chromium.org/12314155/diff/30036/src/ia32/macro-assembler-...
> src/ia32/macro-assembler-ia32.cc:1217: ExternalReference allocation_top =
> ((flags & PRETENURE) != 0) ?
> On 2013/03/12 11:28:20, danno wrote:
> > Does it make sense to create two utility functions:
> > 
> > GetAllocationTopReference() 
> > GetAllocationLimitReference() 
> > 
> > That take flags and return ExternalReferences? You use this in multiple
> places,
> > and it will become even more useful when you add old data space support.
> 
> Done.
> 
> https://codereview.chromium.org/12314155/diff/30036/src/macro-assembler.h
> File src/macro-assembler.h (right):
> 
>
https://codereview.chromium.org/12314155/diff/30036/src/macro-assembler.h#new...
> src/macro-assembler.h:54: PRETENURE = 1 << 4
> On 2013/03/12 11:28:20, danno wrote:
> > Do you think it makes sense to call this PRETENURE_OLD_POINTER_SPACE?
> > 
> 
> Done.

I created a class AllocationUtils in macro-assembler.h which provides
GetAllocationTopReference() and GetAllocationLimitReference().

[danno, 2013-03-13 09:08:32]

lgtm

https://codereview.chromium.org/12314155/diff/43001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/12314155/diff/43001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:1641: ((flags & DOUBLE_ALIGNMENT) != 0)) {
Is this really right? Don't you want to ASSERT instead?

https://codereview.chromium.org/12314155/diff/43001/src/macro-assembler.h
File src/macro-assembler.h (right):

https://codereview.chromium.org/12314155/diff/43001/src/macro-assembler.h#new...
src/macro-assembler.h:172: 
tiny nit: two empty lines before and after?

[Hannes Payer (out of office), 2013-03-14 08:18:51]

https://codereview.chromium.org/12314155/diff/43001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/12314155/diff/43001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:1641: ((flags & DOUBLE_ALIGNMENT) != 0)) {
On 2013/03/13 09:08:32, danno wrote:
> Is this really right? Don't you want to ASSERT instead?

Done.

https://codereview.chromium.org/12314155/diff/43001/src/macro-assembler.h
File src/macro-assembler.h (right):

https://codereview.chromium.org/12314155/diff/43001/src/macro-assembler.h#new...
src/macro-assembler.h:172: 
On 2013/03/13 09:08:32, danno wrote:
> tiny nit: two empty lines before and after?

Done.

[Hannes Payer (out of office), 2013-03-14 08:33:15]

Committed patchset #15 manually as r13940 (presubmit successful).