I like this CL much better!
Apart from some structural comments, I'm not sure about the arguments object
handling. Can we defer that part to another CL? That gets the overall .call
inlining already out of the way.
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode7978
src/hydrogen.cc:7978: bool
HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
Does it make sense to have both methods? I'm pretty sure that by having
both, people will start calling the other version, which may not be
correct?
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode7984
src/hydrogen.cc:7984: ) {
Nit: put ) { back on the previous line
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8005
src/hydrogen.cc:8005: // expr could be .call or .apply so it's not safe
to rely on its
I don't think we need this comment...
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8570
src/hydrogen.cc:8570: if (TryInlineApply(known_function, expr,
args_count_no_receiver)) {
Seems like TryIndirectCall splits to apply and call handling, which join
again in HandleIndirectCall. It's a bit weird that it here again has
special handling for Apply. Is it really specific to Apply, or should it
just be named TryInlineIndirectCall?
Or perhaps we can even merge it with TryInlineCall since they don't
behave that different anymore.
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8575
src/hydrogen.cc:8575: HValue* receiver =
environment()->ExpressionStackAt(args_count_no_receiver);
Can you move this code into TryInlineBuiltinMethodCall?
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8581
src/hydrogen.cc:8581: if (TryInlineBuiltinMethodCall(expr, receiver,
map, known_function,
This was already done in BuildFunctionCallCall. Can we avoid this
duplication? Either pass in a flag for the extra behavior, or move this
out to the Apply variant. Well, I guess we first have to think about the
extra behavior tout court... see the comment down below.
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8603
src/hydrogen.cc:8603: !expr->target()->shared()->HasBuiltinFunctionId())
{
What about creating both BuildFunctionCall and BuildFunctionApply, and
using a switch to jump to those? I think you did that in your previous
CL... That makes it a bit more symmetrical.
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8686
src/hydrogen.cc:8686: receiver = is_builtin_call ? receiver :
BuildWrapReceiver(receiver, function);
You can't just skip BuildWrapReceiver for builtins. BuildWrapReceiver
has the required logic to figure out whether it can be skipped or not
(skipped only when it's in strict-mode). Slice is in strict-mode, which
is what you care about most, so that should work out I presume?
https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8710
src/hydrogen.cc:8710: // be allowed after all.
Iek! So what if I do the following?
a = [];
a.push(arguments);
return a;
https://codereview.chromium.org/335683002/
--
--
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/d/optout.
