Fix jump targets on ARM to merge virtual frames (really this time)....

Fix jump targets on ARM to merge virtual frames (really this time).
Use the jump targets to move the first deferred code to not flush
registers in the inlined case (KeyedLoad).

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

[Søren Thygesen Gjesse, 2010-05-27 07:06:21]

LGTM

http://codereview.chromium.org/2249002/diff/10002/12001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5421
src/arm/codegen-arm.cc:5421: void DeferredReferenceGetNamedValue::Generate() {
Maybe add the expected height assert as in other functions.

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5425
src/arm/codegen-arm.cc:5425: ASSERT(receiver_.is(r0) || receiver_.is(r1));
Maybe we should have an AssertIsTOSRegister.

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5620
src/arm/codegen-arm.cc:5620: deferred->BindExit();
This deserves a comment saying that the receiver register now has the result.

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5867
src/arm/codegen-arm.cc:5867: if (!persist_after_get_) {
This is nasty. Shouldn't we aim at getting rid of References?

http://codereview.chromium.org/2249002/diff/10002/12006
File src/arm/virtual-frame-arm.cc (right):

http://codereview.chromium.org/2249002/diff/10002/12006#newcode166
src/arm/virtual-frame-arm.cc:166: if (cond == al) {
This relies on the fact that the frame is only merged on backward branches?

[Erik Corry, 2010-05-27 13:05:44]

http://codereview.chromium.org/2249002/diff/10002/12001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5421
src/arm/codegen-arm.cc:5421: void DeferredReferenceGetNamedValue::Generate() {
On 2010/05/27 07:06:21, Søren Gjesse wrote:
> Maybe add the expected height assert as in other functions.

Done.

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5425
src/arm/codegen-arm.cc:5425: ASSERT(receiver_.is(r0) || receiver_.is(r1));
On 2010/05/27 07:06:21, Søren Gjesse wrote:
> Maybe we should have an AssertIsTOSRegister.

Instead I asserted that the receiver was not a scratch register, since that is
all that matters.

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5620
src/arm/codegen-arm.cc:5620: deferred->BindExit();
On 2010/05/27 07:06:21, Søren Gjesse wrote:
> This deserves a comment saying that the receiver register now has the result.

Done.

http://codereview.chromium.org/2249002/diff/10002/12001#newcode5867
src/arm/codegen-arm.cc:5867: if (!persist_after_get_) {
On 2010/05/27 07:06:21, Søren Gjesse wrote:
> This is nasty. Shouldn't we aim at getting rid of References?

Discussed offline.

http://codereview.chromium.org/2249002/diff/10002/12006
File src/arm/virtual-frame-arm.cc (right):

http://codereview.chromium.org/2249002/diff/10002/12006#newcode166
src/arm/virtual-frame-arm.cc:166: if (cond == al) {
On 2010/05/27 07:06:21, Søren Gjesse wrote:
> This relies on the fact that the frame is only merged on backward branches?

No.  If we are merging with a cond of 'al' then either this is a fall-through
merge in which case we need to set the top_of_stack_state_ correctly or it's a
prequel to an unconditional branch in which case the virtual frame will be
invalidated and it doesn't matter what we set its top of stack state to.

On the other hand, for a conditional merge the fall-through code doesn't have a
changed top of stack state since all the state changing instructions were
conditional.

I'll add a comment.