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(),
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-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
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-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);
+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-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.
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-ia32.cc#newcode1460
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-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) {
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.
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.
