Change implementation of eval to make an exact distinction between direct eva...

Change implementation of eval to make an exact distinction between direct eval and aliased eval.

Committed: http://code.google.com/p/v8/source/detail?r=860

[Kasper Lund, 2008-11-26 11:59:19]

Overall the change looks sound, but I think you should change it so that you
only have one runtime call per possibly direct eval. Comments:

http://codereview.chromium.org/12673/diff/1/9
File src/ast.h (right):

http://codereview.chromium.org/12673/diff/1/9#newcode868
Line 868: enum EvalType {
Do you really want this in all Calls? How about subclassing Call (something like
CallEval) and putting this behaviour in there?

http://codereview.chromium.org/12673/diff/1/15
File src/codegen-arm.cc (right):

http://codereview.chromium.org/12673/diff/1/15#newcode2323
Line 2323: Label after_call;
Move after_call declaration and binding to inside the then part of the if
(node->eval_type...).

http://codereview.chromium.org/12673/diff/1/15#newcode2329
Line 2329: __ CallRuntime(Runtime::kInDirectEval, 1);
How about letting the runtime call return a function that you just need to call?
In case of a direct eval, you would get some new dynamically generated function
back, but in the other cases you would just get the global.eval function back.
This avoids two out of three runtime calls and also the string type testing. The
only slight complication is dealing with the arguments and the receiver on the
stack, but that should be doable.

http://codereview.chromium.org/12673/diff/1/15#newcode2463
Line 2463: __ bind(&after_call);
Move inside if then-part.

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

http://codereview.chromium.org/12673/diff/1/20#newcode2693
Line 2693: Label after_call;
See my comments for the ARM code.

http://codereview.chromium.org/12673/diff/1/17
File src/macro-assembler-arm.cc (right):

http://codereview.chromium.org/12673/diff/1/17#newcode746
Line 746: void MacroAssembler::GetInstanceType(Register holder) {
Wouldn't it be better to pass in a Label we could jump to for non heap objects?
It seems like an overkill to materialize INVALID_TYPE and SMI_TYPE ever.

http://codereview.chromium.org/12673/diff/1/17#newcode750
Line 750: tst(holder, Operand(kFailureTagMask));
You shouldn't expect to see failure tagged pointers ever. I think this code
should be removed or at least hidden behind the debug-code flag. It shouldn't
happen.

http://codereview.chromium.org/12673/diff/1/7
File src/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/12673/diff/1/7#newcode702
Line 702: void MacroAssembler::GetInstanceType(Register holder) {
See comments for ARM.

http://codereview.chromium.org/12673/diff/1/19
File src/parser.cc (right):

http://codereview.chromium.org/12673/diff/1/19#newcode659
Line 659: 
Intentional?

http://codereview.chromium.org/12673/diff/1/19#newcode2643
Line 2643: for (Scope* scope = top_scope_;
Move this code to the Scope implementation; maybe even rename the current
Scope::Lookup to Scope::LookupLocal and call your new code Scope::Lookup.

[Mads Ager (chromium), 2008-11-26 12:12:46]

I have a couple of comments on top of Kasper's.  I think we need to be careful
with evaluation order here.

http://codereview.chromium.org/12673/diff/1/15
File src/codegen-arm.cc (right):

http://codereview.chromium.org/12673/diff/1/15#newcode2323
Line 2323: Label after_call;
Move this new code before the "Fast-case" comment and comment on what it is
doing.

http://codereview.chromium.org/12673/diff/1/15#newcode2328
Line 2328: Load(function);
Does this give us the correct evaluation order?  Shouldn't we always load the
arguments first and then load the function?  Loading the arguments might change
what the function should resolve to.

http://codereview.chromium.org/12673/diff/1/15#newcode2331
Line 2331: __ b(eq, &not_direct_eval);
Is it OK to branch out here and redo the function call?  You have already
evaluated the function expression, and falling into the next code, you will do
that again?  Since evaluating the function expression might have side-effects we
shouldn't do that.

We should probably handle the call in this code no matter if it is an eval call
or not.

http://codereview.chromium.org/12673/diff/1/6
File src/macro-assembler-arm.h (right):

http://codereview.chromium.org/12673/diff/1/6#newcode191
Line 191: // Takes the holder as an object pointer, finds its instance type
Takes the register that holds the holder object pointer ...

[Mads Ager (chromium), 2008-11-27 11:18:08]

If we can perform the runtime call without removing the arguments from the
stack, we can simplify this quite a bit.  Other than that it LGTM.

http://codereview.chromium.org/12673/diff/401/611
File src/codegen-arm.cc (right):

http://codereview.chromium.org/12673/diff/401/611#newcode2438
Line 2438: if (FLAG_debug_info) RecordStatementPosition(node);
RecordStatementPosition checks the debug_info flag, so no need to do it here. 
The rest of the code just calls without the check.

http://codereview.chromium.org/12673/diff/401/611#newcode2447
Line 2447: __ CallRuntime(Runtime::kResolvePossiblyDirectEval, args->length() +
1);
We should try to avoid copying all the arguments to the heap and then
immediately copying them back to the stack.  Can we make a runtime call that
does not touch the arguments and then patch the function and receiver when we
return?

http://codereview.chromium.org/12673/diff/401/611#newcode2466
Line 2466: Label args_0, args_1, args_n, after_call;
We should be able to just do

CallFunctionStub stub(args->length()) 
__ CallStub(&stub);

here.

That would certainly be the case if we can do the runtime call without popping
the arguments.

http://codereview.chromium.org/12673/diff/401/615
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/12673/diff/401/615#newcode2839
Line 2839: void CodeGenerator::VisitCallEval(CallEval* node) {
See comments to ARM version.

[Mads Ager (chromium), 2008-11-27 13:46:02]

LGTM

http://codereview.chromium.org/12673/diff/651/415
File src/codegen-arm.cc (right):

http://codereview.chromium.org/12673/diff/651/415#newcode2434
Line 2434: Comment cmnt(masm_, "[ Call");
CallEval?

http://codereview.chromium.org/12673/diff/651/419
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/12673/diff/651/419#newcode2840
Line 2840: Comment cmnt(masm_, "[ Call");
CallEval?