First round of comments, from a high-level point of view it looks good
already.
I like this change. Just reviewed ia32 and the platform-independent stuff so
far.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/heap.h
File src/heap.h (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/heap.h#newcode229
src/heap.h:229: V(fixed_array_length_field_symbol, "%length")
\
Let's just call this length_field_symbol, the symbol is agnostic to the
type it is used in.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen-instructions.h#newcode4132
src/hydrogen-instructions.h:4132: return (!object->IsAllocateObject() &&
!object->IsAllocate() &&
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.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen-instructions.h#newcode5116
src/hydrogen-instructions.h:5116: CAN_ALLOCATE_IN_OLD_SPACE = 1 << 1,
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.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.cc#newcode985
src/hydrogen.cc:985:
store_map->SetSideEffectDominator(kChangesNewSpacePromotion, elements);
Since we are doing GVN we shouldn't need to set the dominator manually,
let's drop this.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.cc#newcode994
src/hydrogen.cc:994:
store_length->SetSideEffectDominator(kChangesNewSpacePromotion,
elements);
Likewise.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h
File src/hydrogen.h (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#newcode854
src/hydrogen.h:854: class LoopBodyBuilder {
The LoopBodyBuilder seems to be left-over, drop it.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#newcode860
src/hydrogen.h:860: class CopyElementLoopBodyBuilder;
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#newcode916
src/hydrogen.h:916: HValue* IntegerConstant(int value);
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.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#newcode995
src/hydrogen.h:995: friend class CopyElementLoopBodyBuilder;
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/codegen-ia32.cc#newcode404
src/ia32/codegen-ia32.cc:404:
masm->TestJSArrayForAllocationSiteInfo(edx, edi);
I like this change. But use "__" instead of "masm->" here. And in the
two occurrences below.
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/lithium-ia32.cc#newcode2267
src/ia32/lithium-ia32.cc:2267: LOperand* context =
UseAny(instr->context());
It seems info()->MarkAsDeferredCalling() is missing for LAllocateObject.
https://chromiumcodereview.appspot.com/11659022/diff/48001/test/mjsunit/regress/generated-transition-stub.js
File test/mjsunit/regress/generated-transition-stub.js (right):
https://chromiumcodereview.appspot.com/11659022/diff/48001/test/mjsunit/regress/generated-transition-stub.js#newcode1
test/mjsunit/regress/generated-transition-stub.js:1: // Copyright 2013
the V8 project authors. All rights reserved.
Not really a regression test, let's move it to either test/mjsunit or
test/mjsunit/compiler.
https://chromiumcodereview.appspot.com/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.
