Please take another look. Note that I added a flag to disable the new
functionality.
https://codereview.chromium.org/11659022/diff/48001/src/heap.h
File src/heap.h (right):
https://codereview.chromium.org/11659022/diff/48001/src/heap.h#newcode229
src/heap.h:229: V(fixed_array_length_field_symbol, "%length")
\
On 2013/01/31 13:15:10, Michael Starzinger wrote:
Let's just call this length_field_symbol, the symbol is agnostic to
the type it
is used in.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen-instructions.h#newcode4132
src/hydrogen-instructions.h:4132: return (!object->IsAllocateObject() &&
!object->IsAllocate() &&
On 2013/01/31 13:15:10, Michael Starzinger wrote:
This only holds if HAllocate guarantees that it allocates in
new-space. Since
HAllocate will be flexible enough to also allocate in old-space, we
need an
additional predicate like HAllocate::GuaranteedInNewSpace() that we
check here.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen-instructions.h#newcode5116
src/hydrogen-instructions.h:5116: CAN_ALLOCATE_IN_OLD_SPACE = 1 << 1,
On 2013/01/31 13:15:10, Michael Starzinger wrote:
The old-space flag will need to indicate which old-space is targeted
(i.e.
old-data-space, old-pointer-space). Allocation in LO-space is
trickier, because
you cannot do the bulk-allocation trick there.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.cc#newcode985
src/hydrogen.cc:985:
store_map->SetSideEffectDominator(kChangesNewSpacePromotion, elements);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
Since we are doing GVN we shouldn't need to set the dominator
manually, let's
drop this.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.cc#newcode994
src/hydrogen.cc:994:
store_length->SetSideEffectDominator(kChangesNewSpacePromotion,
elements);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
Likewise.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode854
src/hydrogen.h:854: class LoopBodyBuilder {
On 2013/01/31 13:15:10, Michael Starzinger wrote:
The LoopBodyBuilder seems to be left-over, drop it.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode860
src/hydrogen.h:860: class CopyElementLoopBodyBuilder;
On 2013/01/31 13:15:10, Michael Starzinger wrote:
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode860
src/hydrogen.h:860: class CopyElementLoopBodyBuilder;
On 2013/01/31 13:15:10, Michael Starzinger wrote:
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode916
src/hydrogen.h:916: HValue* IntegerConstant(int value);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
There already is a HGraph::GetConstantInt32(), we could reuse that for
the
well-known constants "0" and "1". It makes sure constants are inserted
at the
top of the graph.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode995
src/hydrogen.h:995: friend class CopyElementLoopBodyBuilder;
On 2013/01/31 13:15:10, Michael Starzinger wrote:
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):
https://codereview.chromium.org/11659022/diff/48001/src/ia32/codegen-ia32.cc#newcode404
src/ia32/codegen-ia32.cc:404:
masm->TestJSArrayForAllocationSiteInfo(edx, edi);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
I like this change. But use "__" instead of "masm->" here. And in the
two
occurrences below.
Done.
https://codereview.chromium.org/11659022/diff/48001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://codereview.chromium.org/11659022/diff/48001/src/ia32/lithium-ia32.cc#newcode2267
src/ia32/lithium-ia32.cc:2267: LOperand* context =
UseAny(instr->context());
On 2013/01/31 13:15:10, Michael Starzinger wrote:
It seems info()->MarkAsDeferredCalling() is missing for
LAllocateObject.
Done.
https://codereview.chromium.org/11659022/
--
--
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.
