Emitting the common return sequence in the top-level compiler in one function...

Emitting the common return sequence in the top-level compiler in one function for each platform.

I factored out the code for emitting the return sequence since we had this code duplicated in the top-level compiler.


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

[Kevin Millikin (Chromium), 2009-11-03 11:31:04]

http://codereview.chromium.org/354024/diff/1/5
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/354024/diff/1/5#newcode108
Line 108: { Comment cmnt(masm_, "Return sequence");
You don't need a block inside a block.

I would put this comment only in the case that you are emitting the return
sequence, not in the case that you are jumping to it.

Consider making this a "[ " comment, so the end is marked as well.

http://codereview.chromium.org/354024/diff/1/5#newcode113
Line 113: SetReturnPosition(function_);
This can't be right, can it?  It says that this return corresponds to the source
location of the end of the function, even for return statements.

I think you need to call SetReturnPosition before the 'return unefined' call to
EmitReturnSequence, and SetStatementPosition before the other ones.

Or, consider passing the source position to EmitReturnSequence (then you can
probably get rid of SetReturnPosition completely).

http://codereview.chromium.org/354024/diff/1/2
File src/fast-codegen.h (right):

http://codereview.chromium.org/354024/diff/1/2#newcode59
Line 59: void EmitReturnSequence();
Move this down by all the other "EmitXXX" platform-specific helpers.  Add a
comment that it is platform specific.

[fschneider, 2009-11-03 12:24:30]

http://codereview.chromium.org/354024/diff/1/5
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/354024/diff/1/5#newcode113
Line 113: SetReturnPosition(function_);
On 2009/11/03 11:31:04, Kevin Millikin wrote:
> This can't be right, can it?  It says that this return corresponds to the
source
> location of the end of the function, even for return statements.
> 
> I think you need to call SetReturnPosition before the 'return unefined' call
to
> EmitReturnSequence, and SetStatementPosition before the other ones.
> 
> Or, consider passing the source position to EmitReturnSequence (then you can
> probably get rid of SetReturnPosition completely).

You may be right. I was not sure, what would be correct. What if the "return
undefined" is dead code? We would have to record the function end position in
VisitReturn then, right? SetStatementPositionis called before evaluating
return-statement exprssion in VisitReturn.

Currently we also do a different thing on ARM and IA-32. I think it should be
consistent on all platforms.

http://codereview.chromium.org/354024/diff/1/2
File src/fast-codegen.h (right):

http://codereview.chromium.org/354024/diff/1/2#newcode59
Line 59: void EmitReturnSequence();
On 2009/11/03 11:31:04, Kevin Millikin wrote:
> Move this down by all the other "EmitXXX" platform-specific helpers.  Add a
> comment that it is platform specific.

Done.

[William Hesse, 2009-11-03 13:18:21]

Kevin's comments need to be addressed, but then it looks good to me.

[fschneider, 2009-11-03 15:07:31]

Changed call to SetReturnPosition: It is called when generating the return
<undefined>, and SetStatementPosition is called in VisitReturn.

[Kevin Millikin (Chromium), 2009-11-03 19:17:59]

http://codereview.chromium.org/354024/diff/4001/1007
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/354024/diff/4001/1007#newcode104
Line 104: EmitReturnSequence();
I actually think it's better to have the signature void EmitReturnSequence(int
pos).  Here you can call:

EmitReturnSequence(function_->end_position());

At the other site you can call

EmitReturnSequence(stmt->statement_pos());

and you can delete the (now) unneeded FastCodeGenarator::SetReturnPosition.

Then you can write

CodeGenerator::RecordPositions(pos)
__ RecordJSReturn();

or even better, make it a one-liner:

__ RecordJSReturn(pos);

in the implementation of EmitReturnSequence.  This is better because: (a) it
doesn't rely on the (undocumented!) precondition that Set...Position is called
before EmitReturnSequence and (b) the recording of positions is right next to
where it is used in RecordJSReturn, instead of in another function.

The second factor is important---it seems like the present confusion about where
source positions should be recorded and for what is due to they way that they
are so far from where they're used.

[fschneider, 2009-11-04 13:01:34]

Updated so that EmitReturnSequence takes the position as an argument: either the
position of a return statement (when called in VisitReturn), or the function end
position (for the "return undefined")

[Søren Thygesen Gjesse, 2009-11-04 13:14:42]

LGTM

http://codereview.chromium.org/354024/diff/7001/7003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/354024/diff/7001/7003#newcode119
Line 119: }
Please the check for return sequence length found in
CodeGenerator::GenerateReturnSequence (codegen-ia32.cc) here as well.

[fschneider, 2009-11-04 14:10:52]

http://codereview.chromium.org/354024/diff/7001/7003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/354024/diff/7001/7003#newcode119
Line 119: }
On 2009/11/04 13:14:42, Søren Gjesse wrote:
> Please the check for return sequence length found in
> CodeGenerator::GenerateReturnSequence (codegen-ia32.cc) here as well.

Done. (also on x64 and ARM)

[Kevin Millikin (Chromium), 2009-11-04 14:22:36]

Excellent.