Shorten live ranges of argument subexpressions.

Shorten live ranges of argument subexpressions.

Before, argument subexpressions were live up until the call that
consumed them.  With this change we end their live ranges as soon as
possible.  This eliminates a write and read when we would previously
spill them to the wrong location, and simplifies the register
allocator's job to not have to consider these subexpressions.

[Kevin Millikin (Chromium), 2010-12-20 13:08:47]

I'm not thrilled with the PushArgument instructions that pop in and out of
existence all over the place.  Cleaning that up would make this a bigger change,
so I've left it fairly explicit and ugly for now.

Before committing I will have to disable the stack trace test or else change the
way we print traces for the inlined functions.

[fschneider, 2010-12-20 16:13:02]

http://codereview.chromium.org/5964005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5964005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:1219: HOperandVector<1> operands_;
There are now 3 types of calls that take an extra operand: CallKeyed,
CallKnownGlobal and CallNew.

Maybe it is better to have a class HCallUnary that contains the common functions
for the operand. It would save some duplication.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode113
src/hydrogen.cc:113: HInstruction* HBasicBlock::GetLastInstruction() {
I'd strongly favor removing this function together with the cached_ instruction
in a separate change. I can't see a real benefit. start() and end() should be
enough, I think.

If a block is finished, we get get end_->previous(). As long as a block is
unfinished, we can update end_ along the way when adding instructions.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode3238
src/hydrogen.cc:3238: default_graph->FinishExit(new HDeoptimize());
Leave out () after HDeoptimize.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode3550
src/hydrogen.cc:3550: default_graph->FinishExit(new HDeoptimize());
Leave out () after HDeoptimize.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode3896
src/hydrogen.cc:3896: // inlined variants (if any).  Otherwise use the default.
Remove a space after .

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4056
src/hydrogen.cc:4056: HSubgraph* body = CreateInlinedSubgraph(env, target,
function, global_receiver);
Long line.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4198
src/hydrogen.cc:4198: HValue* push_left = environment()->ExpressionStackAt(1);
Can you write the following here instead?

ReplaceAndDeleteArguments(environment(), 3);
HValue* right = Pop();
HValue* left = Pop();
Pop();  // Pop receiver.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4334
src/hydrogen.cc:4334: AddInstruction(push_receiver);
PushAndAdd(push_receiver);

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4478
src/hydrogen.cc:4478: Push(push_constructor);
could be shortened to:

PushAndAdd(new HPushArgument(constructor));

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode5173
src/hydrogen.cc:5173: VisitArgumentList(call->arguments());
I'd replace all VisitArgumentList followed by a check for bailout with a macro
like VISIT_ARGUMENTS that includes the if-statement below.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode5224
src/hydrogen.cc:5224: ast_context()->ReturnInstruction(result, call->id());
There seems a lot of duplicated code for all runtime-functions that call a stub
- and there will be more added. Maybe add a helper like BuildStubCall?

http://codereview.chromium.org/5964005/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5964005/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2401: __ mov(Operand(esp, arity *
kPointerSize), eax);
It would be nice to avoid patching in the global receiver.

[Kevin Millikin (Chromium), 2010-12-21 11:16:37]

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode113
src/hydrogen.cc:113: HInstruction* HBasicBlock::GetLastInstruction() {
Strongly agreed.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode3238
src/hydrogen.cc:3238: default_graph->FinishExit(new HDeoptimize());
Thanks.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4056
src/hydrogen.cc:4056: HSubgraph* body = CreateInlinedSubgraph(env, target,
function, global_receiver);
On 2010/12/20 16:13:02, fschneider wrote:
> Long line.

Thanks.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4198
src/hydrogen.cc:4198: HValue* push_left = environment()->ExpressionStackAt(1);
On 2010/12/20 16:13:02, fschneider wrote:
> Can you write the following here instead?
> 
> ReplaceAndDeleteArguments(environment(), 3);
> HValue* right = Pop();
> HValue* left = Pop();
> Pop();  // Pop receiver.

Not really, right now.  ReplaceAndDeleteArguments does not currently change the
environment.

I considered changing the environment, or having it auto drop the pushed
arguments from the environment to make sure that they weren't accidentally used.
 Nothing seems completely satisfying.

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode4334
src/hydrogen.cc:4334: AddInstruction(push_receiver);
On 2010/12/20 16:13:02, fschneider wrote:
> PushAndAdd(push_receiver);

I like to keep them separate.  PushAndAdd is considered harmful :)

I actually don't like it because adding a value to the bailout environment and
inserting an instruction into the Hydrogen instruction graph are nearly two
separate things.  Writing it as a single operation makes it seem like that's
just the way we do it.

I've reviewed code from several different authors that did something like:

PushAndAdd(instruction);
HValue* value = Pop();

http://codereview.chromium.org/5964005/diff/1/src/hydrogen.cc#newcode5224
src/hydrogen.cc:5224: ast_context()->ReturnInstruction(result, call->id());
On 2010/12/20 16:13:02, fschneider wrote:
> There seems a lot of duplicated code for all runtime-functions that call a
stub
> - and there will be more added. Maybe add a helper like BuildStubCall?

Maybe.  It seems premature.  Refactoring is easier if things are explicit at
every site rather than layers and layers of abstractions.

Also we're inconsistent right now about what is "BuildXXX", what is "HandleXXX",
what is "VisitXXX", and what is "AddXXX".  I'd rather not introduce more at this
point.

[Kevin Millikin (Chromium), 2011-02-10 13:07:54]

I'm closing this issue because it's stale.  I'll resubmit it in stages.