X64: Addition binary operation.

X64: Addition binary operation.

[Lasse Reichstein, 2009-06-23 09:31:02]

Largish review.

[Mads Ager (chromium), 2009-06-23 09:55:24]

LGTM, with minor comments.

http://codereview.chromium.org/146022/diff/1/2
File src/assembler.h (right):

http://codereview.chromium.org/146022/diff/1/2#newcode269
Line 269: #if V8_HOST_ARCH_64_BIT
Bill just fixed this, so you should merge with his changes.

http://codereview.chromium.org/146022/diff/1/6
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/146022/diff/1/6#newcode2392
Line 2392: __ movq(kScratchRegister, Immediate(value_));
Can't you do the push of the Immediate Smi directly?

http://codereview.chromium.org/146022/diff/1/6#newcode2424
Line 2424: __ movq(kScratchRegister, Immediate(value_));
Again, no need to move to scratch register.

http://codereview.chromium.org/146022/diff/1/6#newcode2460
Line 2460: __ push(kScratchRegister);
Ditto.

http://codereview.chromium.org/146022/diff/1/6#newcode2495
Line 2495: //// int int_value = smi_value->value();
?

Remove if unused.

http://codereview.chromium.org/146022/diff/1/6#newcode3433
Line 3433: // TODO(X64): Convert number operands to int32 values.
Please put in a call to UNIMPLEMENTED.

http://codereview.chromium.org/146022/diff/1/8
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/146022/diff/1/8#newcode38
Line 38: #define __ ACCESS_MASM(masm())
Merge error - this looks weird. :-)

http://codereview.chromium.org/146022/diff/1/8#newcode152
Line 152: #undef __
Ditto.

[Lasse Reichstein, 2009-06-23 10:21:36]

http://codereview.chromium.org/146022/diff/1/8
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/146022/diff/1/8#newcode38
Line 38: #define __ ACCESS_MASM(masm())
Removed one and changed masm() to masm_.

[William Hesse, 2009-06-23 10:30:15]

LGTM.

http://codereview.chromium.org/146022/diff/1/6
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/146022/diff/1/6#newcode2495
Line 2495: //// int int_value = smi_value->value();
On 2009/06/23 09:55:24, Mads Ager wrote:
> ?
> 
> Remove if unused.

The int_value should be used when more operations are implemented - it won't be
unused.

http://codereview.chromium.org/146022/diff/1/6#newcode3613
Line 3613: __ xor_(rax, rbx);
ASSERT(kSmiTag == 0);

http://codereview.chromium.org/146022/diff/1/6#newcode3865
Line 3865: __ bind(&call_runtime);
Until you think the builtins are compiled and running, I would put an runtime
assert here with an error message, and comment it.
Otherwise, it may be hard to debug.

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

http://codereview.chromium.org/146022/diff/1/7#newcode564
Line 564: if (!resolved) {
This code is very wrong, I think, since InvokeCode no longer leaves the pc_ at 4
bytes past the call address.  Replace it with Unimplemented, or fix it.

[Lasse Reichstein, 2009-06-23 11:00:57]

Could you check the fix?

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

http://codereview.chromium.org/146022/diff/1/7#newcode564
Line 564: if (!resolved) {
Attempted fixed. Please check if it makes sense.

[William Hesse, 2009-06-23 11:10:46]

On 2009/06/23 11:00:57, Lasse Reichstein wrote:
> Could you check the fix?
> 
> http://codereview.chromium.org/146022/diff/1/7
> File src/x64/macro-assembler-x64.cc (right):
> 
> http://codereview.chromium.org/146022/diff/1/7#newcode564
> Line 564: if (!resolved) {
> Attempted fixed. Please check if it makes sense.

Looks right.  Maybe the constant could be the same as the general one used by
the relocator, 
  // Distance between the address of the code target in the call instruction
  // and the return address.  Checked in the debug build.
  static const int Assembler::kTargetAddrToReturnAddrDist = 3 + kPointerSize;