Port a couple more stubs to x64.

Port a couple more stubs to x64.
Fix integer implementation issues.
Enable all tests on x64.
Committed: https://code.google.com/p/dart/source/detail?r=3307

[Ivan Posva, 2012-01-11 00:49:18]

First round of comments.

-Ivan

http://codereview.chromium.org/9114054/diff/1/runtime/vm/cpu_x64.cc
File runtime/vm/cpu_x64.cc (right):

http://codereview.chromium.org/9114054/diff/1/runtime/vm/cpu_x64.cc#newcode44
runtime/vm/cpu_x64.cc:44: __asm {
Please do not put code into the repository that has obviously not been tested.
This should be UNIMPLEMENTED().

http://codereview.chromium.org/9114054/diff/1/runtime/vm/cpu_x64.cc#newcode94
runtime/vm/cpu_x64.cc:94: __asm {
ditto.

[regis, 2012-01-11 01:51:12]

Addressed first comments and added inline allocation of closures and contexts.

Thanks,
Regis

http://codereview.chromium.org/9114054/diff/1/runtime/vm/cpu_x64.cc
File runtime/vm/cpu_x64.cc (right):

http://codereview.chromium.org/9114054/diff/1/runtime/vm/cpu_x64.cc#newcode44
runtime/vm/cpu_x64.cc:44: __asm {
On 2012/01/11 00:49:18, Ivan Posva wrote:
> Please do not put code into the repository that has obviously not been tested.
> This should be UNIMPLEMENTED().

Done. I guess we should use stubs instead of inline assembly.

http://codereview.chromium.org/9114054/diff/1/runtime/vm/cpu_x64.cc#newcode94
runtime/vm/cpu_x64.cc:94: __asm {
On 2012/01/11 00:49:18, Ivan Posva wrote:
> ditto.

Done.

[regis, 2012-01-12 22:08:17]

While waiting for your review, I have completed all 64-bit stubs and marked them
as untested.

[Ivan Posva, 2012-01-12 22:27:31]

Comments on a previous upload. Restarting from the beginning again.

-Ivan

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/date.cc
File runtime/lib/date.cc (right):

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/date.cc#newcode43
runtime/lib/date.cc:43: // See issue 1143.
How about changing the TODO above to TODO(1143) instead?

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:75: const int64_t limit = static_cast<int64_t>(1) << 62;
Please use (static_cast<int64_t>(1)) to get rid of any questions about
precedence between cast and << operators.

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:233: int64_t result_64 =
result_64 => result

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:452: intptr_t result = 0;
How about adding
intptr_t left_value = left.Value();
intptr_t right_value = right.Value();
to avoid many repetitive calls to get the value out of the Smis?

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:474: const int shift_amount =
intptr_t as you are assigning right.Value() to this.

[regis, 2012-01-12 22:53:37]

Thanks

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/date.cc
File runtime/lib/date.cc (right):

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/date.cc#newcode43
runtime/lib/date.cc:43: // See issue 1143.
On 2012/01/12 22:27:31, Ivan Posva wrote:
> How about changing the TODO above to TODO(1143) instead?

Done. Also added a static cast to make the conversion explicit.

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:75: const int64_t limit = static_cast<int64_t>(1) << 62;
On 2012/01/12 22:27:31, Ivan Posva wrote:
> Please use (static_cast<int64_t>(1)) to get rid of any questions about
> precedence between cast and << operators.

Done.

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:233: int64_t result_64 =
On 2012/01/12 22:27:31, Ivan Posva wrote:
> result_64 => result

Done.

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:452: intptr_t result = 0;
On 2012/01/12 22:27:31, Ivan Posva wrote:
> How about adding
> intptr_t left_value = left.Value();
> intptr_t right_value = right.Value();
> to avoid many repetitive calls to get the value out of the Smis?

Done here and in BinaryOpWithTwoSmis.

http://codereview.chromium.org/9114054/diff/8034/runtime/lib/integers.cc#newc...
runtime/lib/integers.cc:474: const int shift_amount =
On 2012/01/12 22:27:31, Ivan Posva wrote:
> intptr_t as you are assigning right.Value() to this.

Done.

[Ivan Posva, 2012-01-13 06:27:40]

http://codereview.chromium.org/9114054/diff/14004/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9114054/diff/14004/runtime/lib/integers.cc#new...
runtime/lib/integers.cc:232: ASSERT(Smi::kBits < 32);  // Do not use this code
in 64-bit mode.
I am a bit concerned how this logic is split up between the two different areas.
Either we handle all multiplication of two Smi inputs here or we leave it at the
bottom. At the moment the logic is spread into too many places.

Personally, I would prefer to handle all operations of two Smis here, but that
means that this helper function needs to change its signature and return a
RawInteger*.

Actually you might want to change the signature in such a way that left and
right are expected to be int64_t with a value that fits in 63-bits. Which would
make the code dispatching below even simpler.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/cpu_x64.cc
File runtime/vm/cpu_x64.cc (right):

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/cpu_x64.cc#newco...
runtime/vm/cpu_x64.cc:28: NoGCScope no_gc;
This is also in the ia32 code, but I was wondering how the destructor for this
stack object is ever called.

Tracing what happens, I realized that since NoGCScope is a StackResource and it
will be unwound by the general unwinding code below. That is rather twisted and
probably needs a comment somewhere. How about adding this here, below and in the
cpu_ia32.cc file?

 // The no_gc StackResource is unwound through the tear down of stack resources
below.
NoGCScope no_gc;

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/cpu_x64.cc#newco...
runtime/vm/cpu_x64.cc:82: // Set up the unhandled exception object as the return
value in EAX
EAX -> RAX

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (left):

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:859: // RBX and RDX are destroyed.
What is the convention regarding these comments. Sometimes they are here,
sometimes they are not.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:283: __ Untested("OptimizeInvokedFunction stub");
I have not reviewed any of the untested stubs in this file.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:786: // Uses R13.
Also uses RAX, RBX.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:828: __ pushq(RBX);
What is in RBX?

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:1324: // Uses RAX, RCX as temporary registers.
At least R13, RDI are also touched.

[regis, 2012-01-13 18:57:29]

Thanks. Please have another look.

-- Regis

http://codereview.chromium.org/9114054/diff/14004/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9114054/diff/14004/runtime/lib/integers.cc#new...
runtime/lib/integers.cc:232: ASSERT(Smi::kBits < 32);  // Do not use this code
in 64-bit mode.
On 2012/01/13 06:27:40, Ivan Posva wrote:
> I am a bit concerned how this logic is split up between the two different
areas.
> Either we handle all multiplication of two Smi inputs here or we leave it at
the
> bottom. At the moment the logic is spread into too many places.
> 
> Personally, I would prefer to handle all operations of two Smis here, but that
> means that this helper function needs to change its signature and return a
> RawInteger*.
> 
> Actually you might want to change the signature in such a way that left and
> right are expected to be int64_t with a value that fits in 63-bits. Which
would
> make the code dispatching below even simpler.

I have merged this helper into its caller, which makes the code cleaner and
easier to understand.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/cpu_x64.cc
File runtime/vm/cpu_x64.cc (right):

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/cpu_x64.cc#newco...
runtime/vm/cpu_x64.cc:28: NoGCScope no_gc;
On 2012/01/13 06:27:40, Ivan Posva wrote:
> This is also in the ia32 code, but I was wondering how the destructor for this
> stack object is ever called.
> 
> Tracing what happens, I realized that since NoGCScope is a StackResource and
it
> will be unwound by the general unwinding code below. That is rather twisted
and
> probably needs a comment somewhere. How about adding this here, below and in
the
> cpu_ia32.cc file?
> 
>  // The no_gc StackResource is unwound through the tear down of stack
resources
> below.
> NoGCScope no_gc; 

Done here, below, and in cpu_ia32.cc.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/cpu_x64.cc#newco...
runtime/vm/cpu_x64.cc:82: // Set up the unhandled exception object as the return
value in EAX
On 2012/01/13 06:27:40, Ivan Posva wrote:
> EAX -> RAX

Done.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (left):

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:859: // RBX and RDX are destroyed.
On 2012/01/13 06:27:40, Ivan Posva wrote:
> What is the convention regarding these comments. Sometimes they are here,
> sometimes they are not.

Personally, I think these comments are a bad idea. They can get out of sync
easily and we should not rely on them. Since they are present in the 32-bit
version, I tried to keep them, but I now removed them. What is much more useful
is to specify what registers need to be preserved by the stub. I did that in the
64-bit version, already.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:283: __ Untested("OptimizeInvokedFunction stub");
On 2012/01/13 06:27:40, Ivan Posva wrote:
> I have not reviewed any of the untested stubs in this file.

Fair enough.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:786: // Uses R13.
On 2012/01/13 06:27:40, Ivan Posva wrote:
> Also uses RAX, RBX.

Removed comment. See below.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:828: __ pushq(RBX);
On 2012/01/13 06:27:40, Ivan Posva wrote:
> What is in RBX?

Added comment.

http://codereview.chromium.org/9114054/diff/14004/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:1324: // Uses RAX, RCX as temporary registers.
On 2012/01/13 06:27:40, Ivan Posva wrote:
> At least R13, RDI are also touched.

Removed comment. See above.

[Ivan Posva, 2012-01-13 19:55:39]

LGTM -ip