Inline array loads in loops directly in the code instead of always...

Inline array loads in loops directly in the code instead of always
calling a stub.  The map to check against is unknown when generating
the code, so we patch the map check in the IC initialization code.

Loop nesting is currently not tracked on ARM.  I'll file feature
request bug reports for implementing this on ARM and add the number to
the TODOs before I commit.

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

[Kasper Lund, 2008-12-22 09:52:13]

LGTM.

http://codereview.chromium.org/16409/diff/212/216
File src/builtins-ia32.cc (right):

http://codereview.chromium.org/16409/diff/212/216#newcode622
Line 622: __ nop();
Is this really necessary? The next instruction here is always an 'add'
instruction. Maybe just document that it has to be that way.

http://codereview.chromium.org/16409/diff/212/222
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16409/diff/212/222#newcode3904
Line 3904: __ sar(eax, kSmiTagSize);
Maybe we should consider keeping the FixedArray length as a smi instead of the
untagging in the fast case?

http://codereview.chromium.org/16409/diff/212/222#newcode3934
Line 3934: __ nop();
I guess here the nop is really needed because the following Push may be
peep-holed away. Maybe you should add that to the comment, because if you just
look at the code it seems like a non-test instruction will always follow the
call.

http://codereview.chromium.org/16409/diff/212/219
File src/d8.cc (right):

http://codereview.chromium.org/16409/diff/212/219#newcode371
Line 371: ::printf("| Name                                   | Value      
|\n");
Maybe 'Value' should be right justified?

[Mads Ager (chromium), 2008-12-22 12:59:11]

http://codereview.chromium.org/16409/diff/212/216
File src/builtins-ia32.cc (right):

http://codereview.chromium.org/16409/diff/212/216#newcode622
Line 622: __ nop();
On 2008/12/22 09:52:13, Kasper Lund wrote:
> Is this really necessary? The next instruction here is always an 'add'
> instruction. Maybe just document that it has to be that way.

This is definitely not necessary.  I alternated between having it there and not
a couple of times.  I'll remove the nop and update the comment to state that it
is important that the next instruction is not a test instruction.

http://codereview.chromium.org/16409/diff/212/222
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16409/diff/212/222#newcode3904
Line 3904: __ sar(eax, kSmiTagSize);
On 2008/12/22 09:52:13, Kasper Lund wrote:
> Maybe we should consider keeping the FixedArray length as a smi instead of the
> untagging in the fast case?

That would be an interesting experiment.  I'll try that in another changelist.

http://codereview.chromium.org/16409/diff/212/222#newcode3934
Line 3934: __ nop();
On 2008/12/22 09:52:13, Kasper Lund wrote:
> I guess here the nop is really needed because the following Push may be
> peep-holed away. Maybe you should add that to the comment, because if you just
> look at the code it seems like a non-test instruction will always follow the
> call.

Yes, exactly.  I'll update the comment to make it clear that the push is not
necessarily emitted.

[iposva, 2008-12-22 18:59:15]

two small additional comments...

-Ivan

http://codereview.chromium.org/16409/diff/212/222
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16409/diff/212/222#newcode3887
Line 3887: // if not contextual) and that it has the expected map.
I think it would improve the readability if was clear that if var is not NULL,
that it is the global context. In other areas in this code we
ASSERT(var->is_gobal()) if var is not NULL.

http://codereview.chromium.org/16409/diff/212/222#newcode3934
Line 3934: __ nop();
On 2008/12/22 12:59:11, Mads Ager wrote:
> On 2008/12/22 09:52:13, Kasper Lund wrote:
> > I guess here the nop is really needed because the following Push may be
> > peep-holed away. Maybe you should add that to the comment, because if you
just
> > look at the code it seems like a non-test instruction will always follow the
> > call.
> 
> Yes, exactly.  I'll update the comment to make it clear that the push is not
> necessarily emitted.
> 
> 

Alternatively you could bind a label after the push if you really need the push
to be emitted. That way the the push cannot be combined with a potential pop.