String check for binary add on x64 and ARM...

String check for binary add on x64 and ARM

The checking for strings when performing binary add was missing on x64 and ARM. This is a prerequisite for adding string add in generated code for these two platforms.

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

[Lasse Reichstein, 2009-12-04 08:07:12]

LGTM

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

http://codereview.chromium.org/465028/diff/1/3#newcode5207
src/arm/codegen-arm.cc:5207: __ mov(r0, Operand(1));  // Set number of
arguments.
Why is number of arguments 1 here, not 2?

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

http://codereview.chromium.org/465028/diff/1/2#newcode7732
src/x64/codegen-x64.cc:7732: __ InvokeBuiltin(Builtins::ADD, JUMP_FUNCTION);
Still calling it first?

http://codereview.chromium.org/465028/diff/1/2#newcode7739
src/x64/codegen-x64.cc:7739: is_smi = __ CheckSmi(rax);
Use masm->CheckSmi(rax) (or masm() or masm_ or whatever __ expands to here). The
__ macro for doing coverage testing doesn't allow returning a value.

http://codereview.chromium.org/465028/diff/1/2#newcode7745
src/x64/codegen-x64.cc:7745: is_smi = __ CheckSmi(rdx);
Ditto on __.

http://codereview.chromium.org/465028/diff/1/2#newcode7760
src/x64/codegen-x64.cc:7760: is_smi = __ CheckSmi(rdx);
And again.

[Søren Thygesen Gjesse, 2009-12-04 09:57:36]

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

http://codereview.chromium.org/465028/diff/1/3#newcode5207
src/arm/codegen-arm.cc:5207: __ mov(r0, Operand(1));  // Set number of
arguments.
On 2009/12/04 08:07:12, Lasse Reichstein wrote:
> Why is number of arguments 1 here, not 2?

Good, question. It looks as if passing number of arguments in r0 in no longer
used. Will remove all these in separate changelist.

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

http://codereview.chromium.org/465028/diff/1/2#newcode7732
src/x64/codegen-x64.cc:7732: __ InvokeBuiltin(Builtins::ADD, JUMP_FUNCTION);
On 2009/12/04 08:07:12, Lasse Reichstein wrote:
> Still calling it first?

Certainly not - good catch - removed. After removing this it turned out that I
had first and second argument mixed up in the code below.

http://codereview.chromium.org/465028/diff/1/2#newcode7739
src/x64/codegen-x64.cc:7739: is_smi = __ CheckSmi(rax);
On 2009/12/04 08:07:12, Lasse Reichstein wrote:
> Use masm->CheckSmi(rax) (or masm() or masm_ or whatever __ expands to here).
The
> __ macro for doing coverage testing doesn't allow returning a value.

__ -> masm-> (masm passed as parameter).

http://codereview.chromium.org/465028/diff/1/2#newcode7745
src/x64/codegen-x64.cc:7745: is_smi = __ CheckSmi(rdx);
On 2009/12/04 08:07:12, Lasse Reichstein wrote:
> Ditto on __.

Done.

http://codereview.chromium.org/465028/diff/1/2#newcode7760
src/x64/codegen-x64.cc:7760: is_smi = __ CheckSmi(rdx);
On 2009/12/04 08:07:12, Lasse Reichstein wrote:
> And again.

Done.