Proper support for deopt_every_n_times option on the x64 and ARM architectures.

Proper support for deopt_every_n_times option on the x64 and ARM architectures.

R=danno@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=17564

[vincent.belliard.fr, 2013-11-07 08:43:23]

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

https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.c...
src/arm/lithium-codegen-arm.cc:773: Register scratch = scratch0();
May be you can leave the case FLAG_deopt_every_n_times == 1. It really generates
less code for this special case.

https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.c...
src/arm/lithium-codegen-arm.cc:778: __ mov(scratch, Operand::Zero());
You can also make this mov conditional (you must negate the condition).

https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.c...
src/arm/lithium-codegen-arm.cc:799: // to use 'msr' and 'mrs' instructions here,
but they are not supported by
mrs and msr are available (assembler-arm.h line 1029).

[Jarin, 2013-11-07 09:10:41]

Thanks for the code review! Responses inline.

On 2013/11/07 08:43:23, vincent.belliard.fr wrote:
> https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.cc
> File src/arm/lithium-codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.c...
> src/arm/lithium-codegen-arm.cc:773: Register scratch = scratch0();
> May be you can leave the case FLAG_deopt_every_n_times == 1. It really
generates
> less code for this special case.

I was considering this, but if you want to deoptimize every time, then a few
extra instructions should not matter much. Moreover, I am told this is only used
in fuzzing tests, so performance is not really important here. Or is there a use
case I am not aware of?

>
https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.c...
> src/arm/lithium-codegen-arm.cc:778: __ mov(scratch, Operand::Zero());
> You can also make this mov conditional (you must negate the condition).

I am wondering if it would make any difference in performance. If yes, which
way? (Not that it matters, the code is for correctness testing).

>
https://codereview.chromium.org/62293002/diff/1/src/arm/lithium-codegen-arm.c...
> src/arm/lithium-codegen-arm.cc:799: // to use 'msr' and 'mrs' instructions
here,
> but they are not supported by
> mrs and msr are available (assembler-arm.h line 1029).

Yes, they are available in the assembler, but our ARM simulator
(simulator-arm.cc) does not know them,  Simulator::DecodeType01() fails at
simulator-arm.cc:2247.

[Sven Panne, 2013-11-07 09:16:05]

On 2013/11/07 09:10:41, jarin wrote:
> [...] I was considering this, but if you want to deoptimize every time, then a
few
> extra instructions should not matter much. Moreover, I am told this is only
used
> in fuzzing tests, so performance is not really important here. Or is there a
use
> case I am not aware of?

Nope, you're right: Performance and code size don't matter at all with this
flag, it is just there to find bugs in our deoptimization framework. Please
don't do anything fancy here, keep the code on all platforms as similar as
possible.

[Weiliang, 2013-11-07 09:20:10]

https://codereview.chromium.org/62293002/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/62293002/diff/1/src/x64/lithium-codegen-x64.c...
src/x64/lithium-codegen-x64.cc:657: Operand count_operand = __
ExternalOperand(count, kScratchRegister);
drive-by comments 
use masm()-> instead of __ here for ExternalOperand

[danno, 2013-11-07 10:57:11]

lgtm

[Jarin, 2013-11-07 13:09:55]

Committed patchset #3 manually as r17564 (presubmit successful).

[Weiliang, 2013-11-08 02:33:02]

On 2013/11/07 13:09:55, jarin wrote:
> Committed patchset #3 manually as r17564 (presubmit successful).

This CL leads to assert failures on x64
tools/run-tests.py --arch x64 --mode debug --extra-flags="--deopt-every-n-times
1000000".
I will fix it in my https://codereview.chromium.org/47533002/