Implement inlining of instanceof tests on ARM.

Implement inlining of instanceof tests on ARM.

[Søren Thygesen Gjesse, 2010-12-20 13:03:25]

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2908: const Register result = r0;
I don't think an alias for the result is required (when it is r0) - just use r0
there.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2917: // and type into scratch
How about implementing IsObjectJSObjectType in the ARM macro assembler as well?

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2932: if (!args_in_registers()) {
Use Drop from the ARM macro assembler to drop two elements from the stack
(several times below).

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2935: }
Use Ret from the ARM macro assembler. I suggest you add a Ret(int drop) function
to the ARM macro assembler to combine the Drop() and Ret().

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2938: __ bind(&miss);
You should use the register aliases created throughout all the code.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2988: __ tst(function, Operand(kSmiTagMask));
The ARM macro assembler has BranchOnSmi.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3005: __ tst(object, Operand(kSmiTagMask));
The ARM macro assembler has BranchOnNotSmi.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3015: // String values are not instances of anything.
Please implement IsObjectStringType in ARM macro assembler.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3021: __
IncrementCounter(&Counters::instance_of_stub_false_string,
You can leave out all the counters. Then you don't need scratch2.

[Søren Thygesen Gjesse, 2010-12-20 13:07:10]

Some more comments, forgot the last two files.

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-arm.cc#newcode1319
src/arm/lithium-arm.cc:1319: new LInstanceOf(UseFixed(instr->left(), r1),
UseFixed(instr->right(), r0));
Long line.

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

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1340: // we expect object and function in
registers r1 and r0.
Please start comment with uppercase letter.

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1345: __ tst(r0, r0);
I think this can be done with conditional moves and no branching.

[Karl Klose, 2010-12-21 08:56:37]

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2917: // and type into scratch
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> How about implementing IsObjectJSObjectType in the ARM macro assembler as
well?

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2932: if (!args_in_registers()) {
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> Use Drop from the ARM macro assembler to drop two elements from the stack
> (several times below).

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2935: }
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> Use Ret from the ARM macro assembler. I suggest you add a Ret(int drop)
function
> to the ARM macro assembler to combine the Drop() and Ret().

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2938: __ bind(&miss);
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> You should use the register aliases created throughout all the code.

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:2988: __ tst(function, Operand(kSmiTagMask));
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> The ARM macro assembler has BranchOnSmi.

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3005: __ tst(object, Operand(kSmiTagMask));
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> The ARM macro assembler has BranchOnNotSmi.

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3015: // String values are not instances of anything.
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> Please implement IsObjectStringType in ARM macro assembler.

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3021: __
IncrementCounter(&Counters::instance_of_stub_false_string,
On 2010/12/20 13:03:25, Søren Gjesse wrote:
> You can leave out all the counters. Then you don't need scratch2.

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-arm.cc#newcode1319
src/arm/lithium-arm.cc:1319: new LInstanceOf(UseFixed(instr->left(), r1),
UseFixed(instr->right(), r0));
On 2010/12/20 13:07:10, Søren Gjesse wrote:
> Long line.

Done.

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

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1340: // we expect object and function in
registers r1 and r0.
On 2010/12/20 13:07:10, Søren Gjesse wrote:
> Please start comment with uppercase letter.

Done.

http://codereview.chromium.org/5988003/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1345: __ tst(r0, r0);
On 2010/12/20 13:07:10, Søren Gjesse wrote:
> I think this can be done with conditional moves and no branching.

Done.

[Søren Thygesen Gjesse, 2010-12-21 09:13:28]

LGTM

http://codereview.chromium.org/5988003/diff/15001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/5988003/diff/15001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:2926: __ Ret(args_in_registers()? 0: 2);
Missing space before ? and : (several times below).