Thanks for the review Danno, here are my changes.
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)
On 2013/02/08 13:44:38, danno wrote:
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.
good idea, thanks.
https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#newcode487
src/arm/code-stubs-arm.cc:487: __ bind(&slow_case);
On 2013/02/08 13:44:38, danno wrote:
In the parameterized stub, you'd have to push the flags here before
calling
through.
Done.
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())) {
On 2013/02/08 13:44:38, danno wrote:
Add this as a predicate somewhere?
Done.
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())) {
On 2013/02/08 13:44:38, danno wrote:
Add this as a predicate somewhere?
Done.
https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc#newcode1746
src/arm/full-codegen-arm.cc:1746: length);
On 2013/02/08 13:44:38, danno wrote:
With the parameterized stub, don't push the flags here.
Done.
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) {
On 2013/02/08 13:44:38, danno wrote:
but push them here...
Done.
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) {
On 2013/02/08 13:44:38, danno wrote:
and here...
Done.
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) ||
On 2013/02/08 13:44:38, danno wrote:
But not here...
Done.
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?
On 2013/02/08 13:44:38, danno wrote:
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.
Good point, thx.
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:
On 2013/02/08 13:44:38, danno wrote:
unneeded whitespace change?
Done.
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_; }
On 2013/02/08 13:44:38, danno wrote:
nit: } should be on next line
Done.
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) {
On 2013/02/08 13:44:38, danno wrote:
Maybe also pass this boolean flag around as a AllocationSiteMode, too?
Done. This area felt a little awkward because I had to distinguish
between a higher level decision to "do or not do" the tracking for the
whole literal, and then within the literal keep deciding on a
member-by-member basis.
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5294
src/hydrogen.cc:5294: if (allocation_sites_enabled &&
boilerplate->IsJSArray()) {
On 2013/02/08 13:44:38, danno wrote:
boilerplate->ShouldTrackAllocationInfo instead?, removing much of the
next few
lines.
It's helpful in other places in the code too, further encapsulating the
logic about which kind of arrays should have the feature, thx!
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5370
src/hydrogen.cc:5370: current_block()->LoopNestingDepth() == 0;
On 2013/02/08 13:44:38, danno wrote:
Maybe make this a predicate somewhere since you call it here and in
full code
gen?
I made it a predicate on the FullCodeGenerator and the GraphBuilder,
since the information is structured a bit differently (the loop nesting
depth).
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5492
src/hydrogen.cc:5492: current_block()->LoopNestingDepth() == 0;
On 2013/02/08 13:44:38, danno wrote:
predicate for this
Done.
https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5525
src/hydrogen.cc:5525: mode == TRACK_ALLOCATION_SITE)) {
On 2013/02/08 13:44:38, danno wrote:
Pass mode rather than a boolean
Done.
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.
