Hi Toon, addressed feedback.
I extracted the HForceRepresentation idef into another CL I should land
first.
Thx,
--Michael
https://codereview.chromium.org/55933002/diff/310001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/55933002/diff/310001/src/code-stubs-hydrogen.cc#newcode712
src/code-stubs-hydrogen.cc:712: : JSArrayBuilder::DONT_FILL_WITH_HOLE;
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Technically we don't need to fill this in with the hole. We could just
fill it
in with 0 (memset).
I've added a TODO for this, it's a neat idea.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen-instructions.h#newcode1580
src/hydrogen-instructions.h:1580: virtual int RedefinedOperandIndex() {
return 0; }
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Why is this needed? HForceRepresentation is explicitly eliminated
after
inserting representation changes.
It's done so we can make optimizations based on seeing HConstant in
JSArrayBuilder. Per our discussion, I'll pull this into a separate CL.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1568
src/hydrogen.cc:1568: HValue* new_object = (array_length == 0)
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Remove the parentheses.
Done.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1578
src/hydrogen.cc:1578: HConstant* initial_capacity_node =
Add<HConstant>(initial_capacity);
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Move this down to the case where initial-capacity-node is used.
Done.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1790
src/hydrogen.cc:1790: if (from->ActualValue()->IsConstant() &&
to->ActualValue()->IsConstant()) {
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Please put this is a separate CL (that also more widely applies
ActualValue to
values you expect to be HConstant).
I made another CL that isolates the idef of HForceRepresentation that
was needed. But the wider application is so wide that I'd like to think
about the best solution a bit. My small CL will solve it locally here
and then a follow up will happen.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode2071
src/hydrogen.cc:2071: if (is_inlined_) {
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Is it good enough to use IsStub() rather than is_inlined? Then we
don't need an
enum after all.
Sure is, thanks for the idea. Done.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode2124
src/hydrogen.cc:2124: return const_size;
On 2013/11/11 13:59:28, Toon Verwaest wrote:
I think constant folding should take care of this. (And in other
places where
you added such code, if there are such).
Indeed, thx!
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7234
src/hydrogen.cc:7234: ASSERT(!environment()->ExpressionStackIsEmpty());
On 2013/11/11 13:59:28, Toon Verwaest wrote:
I this ASSERT and the comment necessary? Doesn't the expression below
fail if
ExpressionStackAt(...) is out of bounds? If it doesn't, can we just
make that
fail?
Done.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7246
src/hydrogen.cc:7246: // to avoid creating a packed non-empty array (not
kosher).
On 2013/11/11 13:59:28, Toon Verwaest wrote:
I'd remove the (not kosher) part.
Done.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7247
src/hydrogen.cc:7247: if (argument_count == 1 &&
!IsHoleyElementsKind(kind)) {
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Can't we just do this in BuildAllocateArrayFromLength? That would seem
better
encapsulated.
Hmm, I'd rather not because then the JSArrayBuilder would have to allow
you to change ElementsKind, currently passed in at constructor time.
This is a special case, and I think it's at the right place for now. I
like the semantics that kind becomes fixed once you have a
JSArrayBuilder.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7336
src/hydrogen.cc:7336: inline_ok =
IsHoleyElementsKind(expr->elements_kind());
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Shouldn't we just deopt if the passed length is non-zero?
Yep, done. I also added a test in array-constructor-feedback.js to
ensure this works as expected.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h#newcode1544
src/hydrogen.h:1544: bool is_inlined = false);
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Please make this an enum.
Removed per your other feedback.
https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h#newcode1586
src/hydrogen.h:1586: bool is_inlined_;
On 2013/11/11 13:59:28, Toon Verwaest wrote:
enum
Removed the member.
https://codereview.chromium.org/55933002/diff/310001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/55933002/diff/310001/src/objects-inl.h#newcode1380
src/objects-inl.h:1380: default:
On 2013/11/11 13:59:28, Toon Verwaest wrote:
No need to add a default case if all cases are handled. Actually
better to not
add a default case, so the compiler will complain when a new,
unsupported case
is added to that enum.
Done.
https://codereview.chromium.org/55933002/diff/310001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/55933002/diff/310001/src/objects.h#newcode8109
src/objects.h:8109: value |= (1 << 16);
On 2013/11/11 13:59:28, Toon Verwaest wrote:
Remove the line above.
oops!
https://codereview.chromium.org/55933002/
--
--
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.
