https://codereview.chromium.org/12314155/diff/12001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/12314155/diff/12001/src/arm/lithium-codegen-arm.cc#newcode5654
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-arm.h
File src/arm/macro-assembler-arm.h (right):
https://codereview.chromium.org/12314155/diff/12001/src/arm/macro-assembler-arm.h#newcode672
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-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/12314155/diff/12001/src/ia32/lithium-codegen-ia32.cc#newcode5519
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-ia32.cc#newcode5519
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-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
https://codereview.chromium.org/12314155/diff/12001/src/ia32/macro-assembler-ia32.cc#newcode1420
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-ia32.cc#newcode1460
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-x64.cc
File src/x64/macro-assembler-x64.cc (right):
https://codereview.chromium.org/12314155/diff/12001/src/x64/macro-assembler-x64.cc#newcode3691
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.cc#newcode170
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#newcode166
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.
https://codereview.chromium.org/12314155/
--
--
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.
