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. http://codereview.chromium.org/5964005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
