Here some initial comments.
https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#newcode413
src/arm/code-stubs-arm.cc:413: // [sp]: flags (ignored)
I don't think you need to pass flags explicitly. Instead, parameterize
the flags into the MinorKey of the stub, keeping the same calling
convention as before. Create then two versions, one that passes
ALLOCATE_SITE_INFO to the tail call into the runtime, and one that
doesn't. At the callee site, you know which version you need, but you
don't have to pay the extra expense of always pushing/popping the
ignored parameter in the fast case.
https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#newcode487
src/arm/code-stubs-arm.cc:487: __ bind(&slow_case);
In the parameterized stub, you'd have to push the flags here before
calling through.
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1594
src/arm/full-codegen-arm.cc:1594: (loop_depth() != 0 ||
!scope()->is_global_scope())) {
Add this as a predicate somewhere?
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1724
src/arm/full-codegen-arm.cc:1724: (loop_depth() != 0 ||
!scope()->is_global_scope())) {
Add this as a predicate somewhere?
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1746
src/arm/full-codegen-arm.cc:1746: length);
With the parameterized stub, don't push the flags here.
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1750
src/arm/full-codegen-arm.cc:1750: } else if (expr->depth() > 1) {
but push them here...
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1752
src/arm/full-codegen-arm.cc:1752: } else if (length >
FastCloneShallowArrayStub::kMaximumClonedLength) {
and here...
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1755
src/arm/full-codegen-arm.cc:1755:
ASSERT(IsFastSmiOrObjectElementsKind(constant_elements_kind) ||
But not here...
https://codereview.chromium.org/12114054/diff/8001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/12114054/diff/8001/src/arm/lithium-codegen-arm.cc#newcode5609
src/arm/lithium-codegen-arm.cc:5609: // memory, right?
No, I don't think so, since if the array already contained objects at
this level, it must have already been FAST_ELEMENTS of some form, which
would prevent it from transition to double. You can add an assert above
somewhere that ASSERT(original_object->HasFastObjectElements()) and
remove this comment.
https://codereview.chromium.org/12114054/diff/8001/src/handles.h
File src/handles.h (right):
https://codereview.chromium.org/12114054/diff/8001/src/handles.h#newcode329
src/handles.h:329:
unneeded whitespace change?
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen-instructions.h#newcode5171
src/hydrogen-instructions.h:5171: return original_boilerplate_; }
nit: } should be on next line
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5290
src/hydrogen.cc:5290: bool allocation_sites_enabled) {
Maybe also pass this boolean flag around as a AllocationSiteMode, too?
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5294
src/hydrogen.cc:5294: if (allocation_sites_enabled &&
boilerplate->IsJSArray()) {
boilerplate->ShouldTrackAllocationInfo instead?, removing much of the
next few lines.
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5370
src/hydrogen.cc:5370: current_block()->LoopNestingDepth() == 0;
Maybe make this a predicate somewhere since you call it here and in full
code gen?
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5492
src/hydrogen.cc:5492: current_block()->LoopNestingDepth() == 0;
predicate for this
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5525
src/hydrogen.cc:5525: mode == TRACK_ALLOCATION_SITE)) {
Pass mode rather than a boolean
https://codereview.chromium.org/12114054/
--
--
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.
