Optimise the deoptimisation check to improve performance on modern ARM cores....

Optimise the deoptimisation check to improve performance on modern ARM cores.

BUG=none
TEST=none

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

Reverted in r7916.

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

[Søren Thygesen Gjesse, 2011-05-16 07:26:39]

Please look into having the jump table at the end of the generated optimized
code instead of inside the constant pool.

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

http://codereview.chromium.org/7021007/diff/1/src/arm/assembler-arm.cc#newcod...
src/arm/assembler-arm.cc:2649: // Emit the deoptimization jump table.
This emitting of the deopt-jump table in parts inside the constant pool looks a
bit complicated. Also we didn't used to have code in the constant pool.

How about placing the whole jump table at the end of the code instead of
interleaving it inside the constant pools?

We will have to limit the max size of the generated code for optimized
functions, but I don't think that is a problem.

http://codereview.chromium.org/7021007/diff/1/src/arm/assembler-arm.cc#newcod...
src/arm/assembler-arm.cc:2669: ASSERT(is_int24(imm24));
Please use the CodePatcher class for this.

CodePatcher patcher(pc, 1);
patcher.masm()->b(...)

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

http://codereview.chromium.org/7021007/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:605: } else {
The x64 also uses a jump table, which is emitted at the end of the generated
code. It also tries to use the same table slot more than once, as there often
are several deopts to the same entry.

[Alexandre, 2011-05-16 11:32:20]

Thanks for the review Søren.

I did not know about the x64 jump table. I refactored the code to use the same
mechanism, generating the table at the end of the code as you suggested. This is
indeed simpler.

The updated patch should be uploaded soon.

Alexandre

On 2011/05/16 07:26:39, Søren Gjesse wrote:
> Please look into having the jump table at the end of the generated optimized
> code instead of inside the constant pool.
> 
> http://codereview.chromium.org/7021007/diff/1/src/arm/assembler-arm.cc
> File src/arm/assembler-arm.cc (right):
> 
>
http://codereview.chromium.org/7021007/diff/1/src/arm/assembler-arm.cc#newcod...
> src/arm/assembler-arm.cc:2649: // Emit the deoptimization jump table.
> This emitting of the deopt-jump table in parts inside the constant pool looks
a
> bit complicated. Also we didn't used to have code in the constant pool.
> 
> How about placing the whole jump table at the end of the code instead of
> interleaving it inside the constant pools?
> 
> We will have to limit the max size of the generated code for optimized
> functions, but I don't think that is a problem.
> 
>
http://codereview.chromium.org/7021007/diff/1/src/arm/assembler-arm.cc#newcod...
> src/arm/assembler-arm.cc:2669: ASSERT(is_int24(imm24));
> Please use the CodePatcher class for this.
> 
> CodePatcher patcher(pc, 1);
> patcher.masm()->b(...)
> 
> http://codereview.chromium.org/7021007/diff/1/src/arm/lithium-codegen-arm.cc
> File src/arm/lithium-codegen-arm.cc (right):
> 
>
http://codereview.chromium.org/7021007/diff/1/src/arm/lithium-codegen-arm.cc#...
> src/arm/lithium-codegen-arm.cc:605: } else {
> The x64 also uses a jump table, which is emitted at the end of the generated
> code. It also tries to use the same table slot more than once, as there often
> are several deopts to the same entry.

[Rodolph Perfetta, 2011-05-16 18:00:40]

Code rewritten similarly to x64 jumptable.

[Søren Thygesen Gjesse, 2011-05-17 06:16:44]

LGTM, with comments addressed

I like the simplifications!

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

http://codereview.chromium.org/7021007/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:263: // Each entry in the jump table generates
only one instruction.
I have not been able to find anywhere where we ensure that this assert holds. We
do try to not crankshaft compile large functions, so most likely this will never
happen. However I find that it will be better to just abort the crankshaft
compilation if this ever happens. That should mark the function as
not-optimizable, and we will not try to crankshaft compile it again.

http://codereview.chromium.org/7021007/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:625: if (FLAG_trap_on_deopt) __
stop("trap_on_deopt");
Shouldn't this stop still be conditional if cc is not al?

[Alexandre, 2011-05-17 08:10:32]

Thanks for the comments.
The updated patch should be uploaded soon.

Alexandre

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

http://codereview.chromium.org/7021007/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:263: // Each entry in the jump table generates
only one instruction.
Done: Changed the ASSERT to a test aborting the compilation if necesary.
On 2011/05/17 06:16:44, Søren Gjesse wrote:
> I have not been able to find anywhere where we ensure that this assert holds.
We
> do try to not crankshaft compile large functions, so most likely this will
never
> happen. However I find that it will be better to just abort the crankshaft
> compilation if this ever happens. That should mark the function as
> not-optimizable, and we will not try to crankshaft compile it again.

http://codereview.chromium.org/7021007/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:625: if (FLAG_trap_on_deopt) __
stop("trap_on_deopt");
It should. Done.

On 2011/05/17 06:16:44, Søren Gjesse wrote:
> Shouldn't this stop still be conditional if cc is not al?

[Rodolph Perfetta, 2011-05-17 10:16:29]

comments addressed.

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

http://codereview.chromium.org/7021007/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:625: if (FLAG_trap_on_deopt) __
stop("trap_on_deopt");
Yes it should.

[Søren Thygesen Gjesse, 2011-05-17 10:36:31]

LGTM

I will commit it.

[Rodolph Perfetta, 2011-05-17 14:41:05]

We couldn't reproduce the failure, but we believe we identified it from the log
and it should be addressed.

[Søren Thygesen Gjesse, 2011-05-18 06:47:13]

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

http://codereview.chromium.org/7021007/diff/11004/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1223: static int MaxNumPRInfo() { return kMaxNumPRInfo;
}
No need to make accessors for constants - just make it public. However the name
is quite strange, and will not make much sense. After all I don't think we need
it - see later comments.

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

http://codereview.chromium.org/7021007/diff/11004/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:271: // We also need to check that the jump table
will require fewer constant
I think we can avoid this restriction - see below.

http://codereview.chromium.org/7021007/diff/11004/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:284: __ bind(&deopt_jump_table_[i].label);
How about avoiding using the const pool all together by emitting a -4 pc
relative load of pc for each table entry.

ldr pc, [pc, #-4]
<address of deopt table entry 0>
ldr pc, [pc, #-4]
<address of deopt table entry 1>
...

[Alexandre, 2011-05-18 09:13:06]

I addressed these new comments.
The updated patch should be uploaded soon.

Alexandre

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

http://codereview.chromium.org/7021007/diff/11004/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1223: static int MaxNumPRInfo() { return kMaxNumPRInfo;
}
Done. I hesitated to do so as everything was declared together.

Rodolph is working on patch to clean up the relocation information on ARM.
Renaming this is part of the clean-up patch I believe.

On 2011/05/18 06:47:13, Søren Gjesse wrote:
> No need to make accessors for constants - just make it public. However the
name
> is quite strange, and will not make much sense. After all I don't think we
need
> it - see later comments.

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

http://codereview.chromium.org/7021007/diff/11004/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:271: // We also need to check that the jump table
will require fewer constant
Removed.

On 2011/05/18 06:47:13, Søren Gjesse wrote:
> I think we can avoid this restriction - see below.

http://codereview.chromium.org/7021007/diff/11004/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:284: __ bind(&deopt_jump_table_[i].label);
Done.
I also moved back the forced emission of the constant pool in the deferred code
generation.

It puts a heavier burden on the previous check, but that should never be a
problem.
It is also worth noting that - if we ever need it - we could go around the first
constraint by generating jump tables regularly instead of only at the end of the
code.

On 2011/05/18 06:47:13, Søren Gjesse wrote:
> How about avoiding using the const pool all together by emitting a -4 pc
> relative load of pc for each table entry.
> 
> ldr pc, [pc, #-4]
> <address of deopt table entry 0>
> ldr pc, [pc, #-4]
> <address of deopt table entry 1>
> ...

[Rodolph Perfetta, 2011-05-19 12:11:26]

Latest comment addressed.

[Søren Thygesen Gjesse, 2011-05-23 12:47:17]

LGTM

I will land this.