thx for comments Toon. I had to make a change to HForceRepresentation to
restore
a lost optimization in BuildFillElementsWithHole(). And refactoring.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (left):
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#oldcode7284
src/hydrogen.cc:7284: HValue* constructor =
HPushArgument::cast(Top())->argument();
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Why is it ok to merge this with the code above? The code above
contains the
function on the TOS, this code contains an HPushArgument(function) on
the TOS.
Because I changed this whole else block to work like the block above:
push function and args onto the expression stack. Then, if we decide to
make the call, we use PreProcessCall() below to remove from the
expression stack and turn into PushArguments. The reason for that is in
order to decide if the array call is eligible for inlining I need to
examine the expressions.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7187
src/hydrogen.cc:7187: HValue* argument =
environment()->ExpressionStackAt(0);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Top();
Done.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7192
src/hydrogen.cc:7192: int value = constant_argument->Integer32Value();
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Rename value to array_size.
Done.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7203
src/hydrogen.cc:7203: : array_builder.AllocateArray(argument, argument,
true);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Can we replace that "true" with an enum indicating the meaning?
Done. Doing this correctly took me on a refactoring journey into
code-stubs-hydrogen because the block just below (pushing capacity and
length) was essentially duplicated.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7270
src/hydrogen.cc:7270: ASSERT(argument_count <= 5);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Why 5? Can we at least make it a descriptive constant somewhere?
Done.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7278
src/hydrogen.cc:7278: new_object = array_builder.AllocateArray(length,
length, fill_with_hole);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Why do we only fill with holes if the kind is smi? At least add a
comment.
Done. Bailout could occur in the HStoreKeyed if we try to initialize a
smi array with a HeapNumber too big. So we have to leave the array
without garbage if that happens.
Also added a TODO that if all the arguments are constants in smi range
then we can eliminate the hole filling.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7306
src/hydrogen.cc:7306: if (argument_count <= 5) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Why 5?
Why indeed. I eliminated this as any predicate.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7309
src/hydrogen.cc:7309: if (!site->DoNotInlineCall()) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
What about just CanInlineCall() rather than the double negative?
Done.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7325
src/hydrogen.cc:7325: // inline this case.
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Shouldn't we just generate a check whether the argument is 0 and bail
out if
not?
I think it's a lot of trouble for a corner case (never happens in
benchmarks). We'd bail out and then have to set the
AllocationSite::DoNotInline flag for the next time around. But setting
that flag is awkward. The bailout code would have to recognize that it
was called with completely valid arguments (FAST_SMI_ELEMENTS, 0 < index
< kInitialMaxFastElementArray), and that needs to be the signal to set
the DoNotInline flag. Alternatively, the DoNotInline flag could be set
just before the bailout. It just doesn't seem worth it.
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc#newcode5767
src/ia32/code-stubs-ia32.cc:5767: Immediate(2));
On 2013/11/06 16:42:44, Toon Verwaest wrote:
I don't know what the line above means.
Trying to be clever adding 1 to a smi in memory. :p. I've cleaned this
up with some infrastructure that verifies you can add one to any packed
elements kind to get the holey version.
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc#newcode5768
src/ia32/code-stubs-ia32.cc:5768: /*
On 2013/11/06 16:42:44, Toon Verwaest wrote:
leftover comment?
Done.
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc#newcode5915
src/ia32/code-stubs-ia32.cc:5915: __ and_(edx, Immediate(0xffff));
Fixed this up too.
https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h#newcode1373
src/objects-inl.h:1373: if (reason == TENURING) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Can we make this a switch to statically ensure we catch all cases?
Yes, but I'm not happy that I have to return "something" at the end on
some compilers.
https://codereview.chromium.org/55933002/diff/170001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/objects.cc#newcode12752
src/objects.cc:12752: if (SitePointsToLiteral() &&
On 2013/11/06 16:42:44, Toon Verwaest wrote:
nit: merge lines
Done.
https://codereview.chromium.org/55933002/diff/170001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/55933002/diff/170001/src/objects.h#newcode8086
src/objects.h:8086: static const int kElementsKindMask = 0xffff;
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Is there a reason why you don't use BitFields?
Nope, now I do, thx!
https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14667
src/runtime.cc:14667: Handle<AllocationSite> site;
On 2013/11/06 16:42:44, Toon Verwaest wrote:
Can we just declare it below, right above the if where it's
initialized?
Done.
https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14691
src/runtime.cc:14691: if (!site.is_null() && !can_use_type_feedback) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
I find all these !site.is_null() a bit hard to read. Can we reorder
the branches
and/or extract code into separate functions?
I pulled out turning the type_info into an allocationsite to the caller,
and consolidated the two points in the function where I set
site->SetDoNotInlineCall().
I think it looks better even though !site.is_null() still exists.
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.
