Win64 - Allow returning two values from a runtime function.

Win64 - Allow returning two values from a runtime function.
(Not yet fully functional)

[Lasse Reichstein, 2009-09-07 11:16:32]

Check this out.

[Søren Thygesen Gjesse, 2009-09-07 12:22:33]

Drive by comments.

http://codereview.chromium.org/201042/diff/3001/3008
File src/codegen.h (right):

http://codereview.chromium.org/201042/diff/3001/3008#newcode320
Line 320: explicit CEntryDebugBreakStub() : CEntryStub(1) { }
explicit only required when there is one argument, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const...

http://codereview.chromium.org/201042/diff/3001/3019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/201042/diff/3001/3019#newcode6928
Line 6928: // Results returned in rax or in rax+rdx by default.
Maybe add something about this being compiler specific.

http://codereview.chromium.org/201042/diff/3001/3019#newcode7005
Line 7005: if (result_size_ < 2) {
Why not == 1? We only have 1 and 2 right?

[William Hesse, 2009-09-07 14:34:18]

LGTM, with comments.

http://codereview.chromium.org/201042/diff/3001/3015
File src/runtime.cc (right):

http://codereview.chromium.org/201042/diff/3001/3015#newcode4557
Line 4557: 
Why three blank lines?

http://codereview.chromium.org/201042/diff/3001/3015#newcode4560
Line 4560: // A mechanism to return pairs of Object*'s. This is somewhat
Object pointers. pointers to Objects.

http://codereview.chromium.org/201042/diff/3001/3015#newcode4563
Line 4563: // arm platform support this; it is mostly an issue of "coaxing" the
Change this whole comment.  Say that it works on other platforms, and what WIN64
does.

http://codereview.chromium.org/201042/diff/3001/3019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/201042/diff/3001/3019#newcode6926
Line 6926: return (result_size_ < 2) ? 0 : result_size_ * 2;
Why is the key 4 if the size is 2, and 0 if it is 1?

http://codereview.chromium.org/201042/diff/3001/3021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/201042/diff/3001/3021#newcode1044
Line 1044: ASSERT(kFrameAlignment == 2 * kPointerSize);  // Change the padding
if needed.
This assert needs to be changed, to assert that the stack is aligned to the
alignment requirement before making the call.

[Lasse Reichstein, 2009-09-08 11:51:35]

http://codereview.chromium.org/201042/diff/3001/3008
File src/codegen.h (right):

http://codereview.chromium.org/201042/diff/3001/3008#newcode320
Line 320: explicit CEntryDebugBreakStub() : CEntryStub(1) { }
Good catch.
(There was an argument at a point, but it got removed without removing the
explicit).

http://codereview.chromium.org/201042/diff/3001/3015
File src/runtime.cc (right):

http://codereview.chromium.org/201042/diff/3001/3015#newcode4557
Line 4557: 
Indeed. Fixed.

http://codereview.chromium.org/201042/diff/3001/3015#newcode4560
Line 4560: // A mechanism to return pairs of Object*'s. This is somewhat
Done.

http://codereview.chromium.org/201042/diff/3001/3015#newcode4563
Line 4563: // arm platform support this; it is mostly an issue of "coaxing" the
Done.

http://codereview.chromium.org/201042/diff/3001/3019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/201042/diff/3001/3019#newcode6926
Line 6926: return (result_size_ < 2) ? 0 : result_size_ * 2;
Previously, minor key 0 was used for CEntryStub and minor key 1 by
CEntryDebugBreakStub (with the same major key). When generalizing CEntryStub I
decided to reserve all odd numbers for the debug break stub, just to be safe.

http://codereview.chromium.org/201042/diff/3001/3019#newcode6928
Line 6928: // Results returned in rax or in rax+rdx by default.
I'll say something.
It's calling convention specific (so compilers that generate compatible object
files should agree), but we should find a reference that states which calling
conventions are being used.

http://codereview.chromium.org/201042/diff/3001/3019#newcode7005
Line 7005: if (result_size_ < 2) {
I was expecting 0 to be allowed too (for a function returning nothing). We don't
actually have that (since all runtime functions must be able to return failure).
It also opens up for having return structures of sizes larger than two (which
might be needless generality).
It does represent the logic exactly, so I prefer it the way it is.

http://codereview.chromium.org/201042/diff/3001/3021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/201042/diff/3001/3021#newcode1044
Line 1044: ASSERT(kFrameAlignment == 2 * kPointerSize);  // Change the padding
if needed.
Rather, we should move the stack aligning code below this block (it's just
above). 
We do know that the stack is aligned before this block, and since we subtract a
multiple of two times kPointerSize, this ASSERT guarantees that we are aligned
after.

[William Hesse, 2009-09-08 12:43:52]

http://codereview.chromium.org/201042/diff/3001/3021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/201042/diff/3001/3021#newcode1044
Line 1044: ASSERT(kFrameAlignment == 2 * kPointerSize);  // Change the padding
if needed.
On 2009/09/08 11:51:35, Lasse Reichstein wrote:
> Rather, we should move the stack aligning code below this block (it's just
> above). 
> We do know that the stack is aligned before this block, and since we subtract
a
> multiple of two times kPointerSize, this ASSERT guarantees that we are aligned
> after.

We don't know that we are subtracting a multiple of two times kPointerSize,
unless we know that result_size is always even or less than two.