Generate inline code for contextual loads on ARM....

Generate inline code for contextual loads on ARM.

This is an ARM port of http://codereview.chromium.org/3402014 and http://codereview.chromium.org/3537003.

BUG=none
TEST=none

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

[Mads Ager (chromium), 2010-11-18 14:54:47]

Søren, could you have a look at this?

[Søren Thygesen Gjesse, 2010-11-19 09:08:47]

LGTM with the comments addressed.

http://codereview.chromium.org/5140002/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/5140002/diff/1/src/arm/assembler-arm.h#newcode...
src/arm/assembler-arm.h:1085: enum NopMarkerTypes {
I think this enum should be moved to the macro-assembler, and nop/IsNop should
keep taking int argument. I know that will require that some of the debug break
slot stuff is moved there as well.

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

http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc#newcode6840
src/arm/codegen-arm.cc:6840: // This code checks for the_hole_value when we
DontDelete cell.
Some words are missing in "when we DontDelete cell"

http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc#newcode6845
src/arm/codegen-arm.cc:6845: if (is_contextual && is_dont_delete &&
FLAG_debug_code) {
Please move the FLAG_debug_code to the beginning of the condition for clarity.

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

http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc#newcode910
src/arm/ic-arm.cc:910: Assembler::NopMarkerTypes type =
I think you should loose the default value here.

http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc#newcode1009
src/arm/ic-arm.cc:1009: // LoadIC::ClearInlinedVersion will call
PatchInlinedContextualLoad with
Could you please elaborate a bit here on the different instructions generated
for is_dont_delete true/false, and how it is patched.

[Søren Thygesen Gjesse, 2010-11-19 09:12:36]

On 2010/11/19 09:08:47, Søren Gjesse wrote:
> LGTM with the comments addressed.
> 
> http://codereview.chromium.org/5140002/diff/1/src/arm/assembler-arm.h
> File src/arm/assembler-arm.h (right):
> 
>
http://codereview.chromium.org/5140002/diff/1/src/arm/assembler-arm.h#newcode...
> src/arm/assembler-arm.h:1085: enum NopMarkerTypes {
> I think this enum should be moved to the macro-assembler, and nop/IsNop should
> keep taking int argument. I know that will require that some of the debug
break
> slot stuff is moved there as well.
> 
> http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc
> File src/arm/codegen-arm.cc (right):
> 
>
http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc#newcode6840
> src/arm/codegen-arm.cc:6840: // This code checks for the_hole_value when we
> DontDelete cell.
> Some words are missing in "when we DontDelete cell"
> 
>
http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc#newcode6845
> src/arm/codegen-arm.cc:6845: if (is_contextual && is_dont_delete &&
> FLAG_debug_code) {
> Please move the FLAG_debug_code to the beginning of the condition for clarity.
> 
> http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc
> File src/arm/ic-arm.cc (right):
> 
> http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc#newcode910
> src/arm/ic-arm.cc:910: Assembler::NopMarkerTypes type =
> I think you should loose the default value here.
> 
> http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc#newcode1009
> src/arm/ic-arm.cc:1009: // LoadIC::ClearInlinedVersion will call
> PatchInlinedContextualLoad with
> Could you please elaborate a bit here on the different instructions generated
> for is_dont_delete true/false, and how it is patched.

Just an additional idea - mayby you can encode the is_dont_delete hint in the
code marker.

[Alexandre, 2010-11-23 11:23:20]

Thanks for the review.

Martyn should have uploaded the updated patch.

Alexandre

http://codereview.chromium.org/5140002/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/5140002/diff/1/src/arm/assembler-arm.h#newcode...
src/arm/assembler-arm.h:1085: enum NopMarkerTypes {
On 2010/11/19 09:08:47, Søren Gjesse wrote:
> I think this enum should be moved to the macro-assembler, and nop/IsNop should
> keep taking int argument. I know that will require that some of the debug
break
> slot stuff is moved there as well.

Left here as discussed.
Moving this to the Masm was not very convenient because of inlined function and
included files dependencies.

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

http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc#newcode6840
src/arm/codegen-arm.cc:6840: // This code checks for the_hole_value when we
DontDelete cell.
Changed the comment.
On 2010/11/19 09:08:47, Søren Gjesse wrote:
> Some words are missing in "when we DontDelete cell"

http://codereview.chromium.org/5140002/diff/1/src/arm/codegen-arm.cc#newcode6845
src/arm/codegen-arm.cc:6845: if (is_contextual && is_dont_delete &&
FLAG_debug_code) {
On 2010/11/19 09:08:47, Søren Gjesse wrote:
> Please move the FLAG_debug_code to the beginning of the condition for clarity.

Done.

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

http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc#newcode910
src/arm/ic-arm.cc:910: Assembler::NopMarkerTypes type =
On 2010/11/19 09:08:47, Søren Gjesse wrote:
> I think you should loose the default value here.

Done.

http://codereview.chromium.org/5140002/diff/1/src/arm/ic-arm.cc#newcode1009
src/arm/ic-arm.cc:1009: // LoadIC::ClearInlinedVersion will call
PatchInlinedContextualLoad with
I refactored the code following your idea to encode is_dont_delete in the
marker, and we now no longer need this extra if.
On 2010/11/19 09:08:47, Søren Gjesse wrote:
> Could you please elaborate a bit here on the different instructions generated
> for is_dont_delete true/false, and how it is patched.

[Søren Thygesen Gjesse, 2010-11-23 14:00:36]

LGTM with minor issues fixed.

http://codereview.chromium.org/5140002/diff/12001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/5140002/diff/12001/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1186: static bool IsNop(Instr instr, int =
NON_MARKING_NOP);
Missing argument name.

http://codereview.chromium.org/5140002/diff/12001/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/5140002/diff/12001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.h:345: // Return <n> if we have a mov rn rn, else
return -1.
How about assigning this to a local variable type and add

ASSERT(type == -1 || IsNop(type));

[Alexandre, 2010-11-23 17:32:59]

The updated patch should be uploaded soon.

http://codereview.chromium.org/5140002/diff/12001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/5140002/diff/12001/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1186: static bool IsNop(Instr instr, int =
NON_MARKING_NOP);
On 2010/11/23 14:00:37, Søren Gjesse wrote:
> Missing argument name.

Done.

http://codereview.chromium.org/5140002/diff/12001/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/5140002/diff/12001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.h:345: // Return <n> if we have a mov rn rn, else
return -1.
On 2010/11/23 14:00:37, Søren Gjesse wrote:
> How about assigning this to a local variable type and add
> 
> ASSERT(type == -1 || IsNop(type));

Added a local variable and assert.
Retrieving the type already checks for a nop, so I just check the type is indeed
a marker.

[m.m.capewell, 2010-11-23 17:35:15]

Updated patch.

[Søren Thygesen Gjesse, 2010-11-24 09:43:58]

On 2010/11/23 17:35:15, m.m.capewell wrote:
> Updated patch.

LGTM, committed r5883, thank you for the patch.