Implemented one-char cache lookup in generated code.

Implemented one-char cache lookup in generated code.

This speeds up string.charAt(n) and string[n].

[Mads Ager (chromium), 2010-02-26 17:59:06]

LGTM with a couple of comments.

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

http://codereview.chromium.org/660184/diff/1/2#newcode3439
src/arm/codegen-arm.cc:3439: __ add(r1, r1, Operand(r0));
I would prefer to use 

__ add(r1, r1, Operand(r0, LSL, kPointerSizeLog2 - kSmiTagSize)) 

instead of these two instructions. Also makes it more clear to me that you are
adding ((untagged r0) * kPointerSize) to r1.

http://codereview.chromium.org/660184/diff/1/2#newcode3442
src/arm/codegen-arm.cc:3442: __ cmp(r1, Operand(Factory::undefined_value()));
You should use LoadRoot to ip followed by cmp here to use the RootArray instead
of embedding the object in the code.

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

http://codereview.chromium.org/660184/diff/1/6#newcode5605
src/ia32/codegen-ia32.cc:5605: frame_->Dup();
Do you need the Dup here?  You never modify code as far as I can tell, so I
guess you could just Pop it from the frame and push it back if you need it on
the frame.  Then you can just push the result instead of overwriting element
zero on the frame with the result.

http://codereview.chromium.org/660184/diff/1/6#newcode5625
src/ia32/codegen-ia32.cc:5625: __ Set(temp.reg(),
Immediate(Factory::single_character_string_cache()));
Adding a comment here stating that code contains a smi tagged ascii char code
might be good to explain the times_hafl_pointer_size in the Operand below.

http://codereview.chromium.org/660184/diff/1/6#newcode5629
src/ia32/codegen-ia32.cc:5629: __ mov(temp.reg(), Operand(temp.reg(),
Use FieldOperand and drop the "- kHeapObjectTag" on the offset?

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

http://codereview.chromium.org/660184/diff/1/12#newcode3906
src/x64/codegen-x64.cc:3906: __ movq(temp.reg(), Operand(temp.reg(),
FieldOperand to get rid of the "- kHeapObjectTag" in the offset.

http://codereview.chromium.org/660184/diff/1/12#newcode3909
src/x64/codegen-x64.cc:3909: __ Cmp(temp.reg(), Factory::undefined_value());
You should use CompareRoot here to use the root array.

[Vitaly Repeshko, 2010-02-26 20:14:48]

Thanks! All done. Submitted.


-- Vitaly

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

http://codereview.chromium.org/660184/diff/1/2#newcode3439
src/arm/codegen-arm.cc:3439: __ add(r1, r1, Operand(r0));
On 2010/02/26 17:59:06, Mads Ager wrote:
> I would prefer to use 
> 
> __ add(r1, r1, Operand(r0, LSL, kPointerSizeLog2 - kSmiTagSize)) 
> 
> instead of these two instructions. Also makes it more clear to me that you are
> adding ((untagged r0) * kPointerSize) to r1.

Done.

http://codereview.chromium.org/660184/diff/1/2#newcode3442
src/arm/codegen-arm.cc:3442: __ cmp(r1, Operand(Factory::undefined_value()));
On 2010/02/26 17:59:06, Mads Ager wrote:
> You should use LoadRoot to ip followed by cmp here to use the RootArray
instead
> of embedding the object in the code.

Done.

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

http://codereview.chromium.org/660184/diff/1/6#newcode5605
src/ia32/codegen-ia32.cc:5605: frame_->Dup();
On 2010/02/26 17:59:06, Mads Ager wrote:
> Do you need the Dup here?  You never modify code as far as I can tell, so I
> guess you could just Pop it from the frame and push it back if you need it on
> the frame.  Then you can just push the result instead of overwriting element
> zero on the frame with the result.

Well, I tried this approach and couldn't make it work. After you comment I tried
a bit more and found out it requires passing live values along branches. Now it
works (and on x64 as well).

http://codereview.chromium.org/660184/diff/1/6#newcode5625
src/ia32/codegen-ia32.cc:5625: __ Set(temp.reg(),
Immediate(Factory::single_character_string_cache()));
On 2010/02/26 17:59:06, Mads Ager wrote:
> Adding a comment here stating that code contains a smi tagged ascii char code
> might be good to explain the times_hafl_pointer_size in the Operand below.

Done.

http://codereview.chromium.org/660184/diff/1/6#newcode5629
src/ia32/codegen-ia32.cc:5629: __ mov(temp.reg(), Operand(temp.reg(),
On 2010/02/26 17:59:06, Mads Ager wrote:
> Use FieldOperand and drop the "- kHeapObjectTag" on the offset?

Done.

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

http://codereview.chromium.org/660184/diff/1/12#newcode3906
src/x64/codegen-x64.cc:3906: __ movq(temp.reg(), Operand(temp.reg(),
On 2010/02/26 17:59:06, Mads Ager wrote:
> FieldOperand to get rid of the "- kHeapObjectTag" in the offset.

Done.

http://codereview.chromium.org/660184/diff/1/12#newcode3909
src/x64/codegen-x64.cc:3909: __ Cmp(temp.reg(), Factory::undefined_value());
On 2010/02/26 17:59:06, Mads Ager wrote:
> You should use CompareRoot here to use the root array.

Done.