Thanks Hannes, here are comment results.
--Michael
https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc
File src/a64/builtins-a64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc#newcode478
src/a64/builtins-a64.cc:478: if (create_memento) {
Hannes, I've changed this snippet of code to fit the same pattern as the
other three platforms:
if (count_constructions) {
// object filling code for this case.
} else if (create_memento) {
// object filling code for this case.
} else {
// simplest case for filling object fields.
}
There is a line or two of duplication, but I think it's worth having a
uniform logical structure. In some platforms it's "not quite"
duplication, like we'll use register ESI instead of EDI in some operands
in order to save an instruction elsewhere.
https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64.cc#newcode5388
src/a64/code-stubs-a64.cc:5388: // We should either have undefined in
ebx or a valid AllocationSite
On 2014/02/18 16:24:26, Hannes Payer wrote:
x10
Done.
https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm.cc#newcode5276
src/arm/code-stubs-arm.cc:5276: // We should either have undefined in
ebx or a valid AllocationSite
On 2014/02/18 16:24:26, Hannes Payer wrote:
r2
Done.
https://codereview.chromium.org/132963012/diff/210001/src/factory.cc
File src/factory.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/factory.cc#newcode1318
src/factory.cc:1318:
isolate()->heap()->AllocateJSObjectWithAllocationSite(
On 2014/02/18 16:24:26, Hannes Payer wrote:
The refactoring of AllocateJSObject* should happen in a differen CL. I
think we
should keep separate factory methods.
Right on, I've delivered a CL to you with that refactoring, good idea.
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc#newcode8224
src/hydrogen.cc:8224: if (!allocation_site.is_null()) {
On 2014/02/18 16:24:26, Hannes Payer wrote:
When is the allocation_site null?
Wow. Never. Simplifying this. In the past it was more like, "if the call
new site never went megamorphic then it could be null." However, we only
come down this code path anywhere if we are monomorphic.
BTW, two lines in type-info.cc could be simplified,
TypeFeedbackOracle::CallIsMonomorphic() and
TypeFeedbackOracle::CallNewIsMonomorphic(). Those calls were accepting
an AllocationSite in their type vector slot, but now we've separated it
out into a different slot.
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h#newcode1002
src/hydrogen.h:1002: : current_site_(NULL), pretenure_flag_(NOT_TENURED)
{}
On 2014/02/18 16:24:26, Hannes Payer wrote:
indent
Done.
https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc#newcode8304
src/runtime.cc:8304: // TODO(mvstanton): make this a real site
On 2014/02/18 16:24:26, Hannes Payer wrote:
ambiguous comment
Done.
https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc#newcode272
src/x64/builtins-x64.cc:272: }
On 2014/02/18 16:24:26, Hannes Payer wrote:
__ InitializeFieldsWithFiller(rcx, rdi, rdx); outside the if/else?
It's awkward to do it because in the create_memento case
it's called with the arg RSI instead of RDI. (ia32 looks similar).
In general this "filling object fields" code was tricky enough that I
finally opted for a single logical structure to lay out the code at the
expense of some duplication/near-duplication.
https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc#newcode404
src/x64/builtins-x64.cc:404: }
On 2014/02/18 16:24:26, Hannes Payer wrote:
__ bind(&count_incremented); outside of if?
I thought about that a lot, and decided to put it inside because the
only "jumper" to that target is in a "if (create_memento)", and this
means that any other unsanctioned jumper to that address would get a
fast crash at compile time. By sticking it inside that block, someone
without create_memento == true cannot possibly jump to it and succeed in
compiling this code. We compile it at snapshot time so it's a
fast/useful error.
I saw this pattern in other places...I agree it's weird but it's safer.
https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler-x64.cc#newcode4052
src/x64/macro-assembler-x64.cc:4052: bind(&fine);
On 2014/02/18 16:24:26, Hannes Payer wrote:
I don't think this code is needed...
Whoa, thank you, that was me debugging somewhere in there :).
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc#newcode2533
test/cctest/test-heap.cc:2533: " return this;"
On 2014/02/18 16:24:26, Hannes Payer wrote:
remove return this;
Done.
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc#newcode2579
test/cctest/test-heap.cc:2579: // Test that a form of call-new
pretenuring works even without allocation sites.
On 2014/02/18 16:24:26, Hannes Payer wrote:
Test global pretenuring of call new.
Done.
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mementos.cc
File test/cctest/test-mementos.cc (right):
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mementos.cc#newcode86
test/cctest/test-mementos.cc:86: "a;";
On 2014/02/18 16:24:26, Hannes Payer wrote:
a == return a; ?
Done.
https://codereview.chromium.org/132963012/
--
--
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.
