Use direct jump and call instruction for X64 when the deoptimization entries are in the code range

Use direct jump and call instruction for X64 when the deoptimization entries are in the code range.

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

[haitao.feng, 2012-12-14 02:24:06]

This patch set is dependent on https://codereview.chromium.org/11566011/.

[Sven Panne, 2012-12-14 08:20:11]

What problem is this CL trying to solve? It's not really obvious.

Furthermore, I stopped adding comments about architecture-dependent stuff in
architecture-independent code parts at some point. Our architecture support is
already a maintenance nightmare, and we should do *everything* to make it
better, not worse. If the CL is really potentially valuable for more than one
platform, split it up cleanly in arch-dependendent and arch-independent parts.
If it is not, completely hide the changes in src/x64.

https://codereview.chromium.org/11574027/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/11574027/diff/1/src/assembler.cc#newcode296
src/assembler.cc:296: #if !defined(V8_TARGET_ARCH_X64)
Again for this and the rest of the file: No architecture-dependent stuff should
be here.

https://codereview.chromium.org/11574027/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/11574027/diff/1/src/assembler.h#newcode244
src/assembler.h:244: #if defined(V8_TARGET_ARCH_X64)
Similar to the comment in the related CL, I will strongly object any change in
this area which is architecture-dependent.

https://codereview.chromium.org/11574027/diff/1/src/assembler.h#newcode261
src/assembler.h:261: #if defined(V8_TARGET_ARCH_X64)
see above

https://codereview.chromium.org/11574027/diff/1/src/assembler.h#newcode310
src/assembler.h:310: #if defined(V8_TARGET_ARCH_X64)
see above

https://codereview.chromium.org/11574027/diff/1/src/assembler.h#newcode362
src/assembler.h:362: #if defined(V8_TARGET_ARCH_X64)
see above

https://codereview.chromium.org/11574027/diff/1/src/assembler.h#newcode499
src/assembler.h:499: #if !defined(V8_TARGET_ARCH_X64)
see above

https://codereview.chromium.org/11574027/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/11574027/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:303: DEFINE_bool(disable_direct_deopt, false,
If we really need a flag (not sure about this), an "enable-style" flag is
prefered. Double negation is hard... ;-)

[danno, 2012-12-19 17:07:33]

The more I think about this, the more I think this approach is not quite the
right one. Instead of trying to introduce a new type of reloc info for just
deopt entries, I think it makes sense to try to compact all RUNTIME_ENTRYs on
x64 (i.e. all pointers to code in the V8 runtime that can be treated like C/C++
functions), which is how deopts entries are handled on other platforms (see
disassembler.cc). If all RUNTIME_ENTRYs are converted to relative 32-bit
relative jumps, then deopt entries can just be RUNTIME_ENTRY like on other
platforms and they will work "for free". For existing RUNTIME_ENTRY that
currently need to be 64-bits you can provide trampolines in the CodeRange that
jump to the "real" runtime entry points.

[haitao.feng, 2012-12-20 06:46:07]

danno, thanks for your review.

For existing RUNTIME_ENTRY that currently need to be 64-bits, do you mean we
need EXTERNAL_REFERENCE relocation info for them? For X64, they are:
1)    __ movq(kScratchRegister,
            FUNCTION_ADDR(Runtime::PerformGC),
            RelocInfo::RUNTIME_ENTRY);
    __ call(kScratchRegister); from code-stubs-x64.cc
2)  // Call the api function!
    movq(rax, reinterpret_cast<int64_t>(function_address),
       RelocInfo::RUNTIME_ENTRY);
    call(rax);  from macro-assember-x64.cc

Besides those usage of RUNTIME_ENTRY, the others are for the deoptimization
table entries mentioned in this CL.

If we want to use RUNTIME_ENTRY, instead of introducing DEOPT_ENTRY, the
handling of RUNTIME_ENTRY for X64 is same with DEOPT_ENTRY. The reason is that
the 32-bit delta for X64 could only be computed when we invoke CopyFrom as shown
in this CL as at this time the copied codes and deoptimization entries are both
in code range. The handling of RUNTIME_ENTRY for IA32 is different as the 32-bit
delta could be computed anytime.

[danno, 2012-12-20 10:39:02]

Actually, after looking at INTERNAL_REFERENCES more closely, they seem to be
exactly what you need. The are 32-but relative addresses that get updated
appropriately during code motion during a GC. So the best approach of all may be
to leave existing RUNTIME_ENTRIES as-is, and simply change deopt entries to be
INTERNAL_REFERENCES on x64. This will require a little bit of tweaking of the
x64 assembler, but that should be straight-forward.

[haitao.feng, 2012-12-21 03:16:25]

Thanks for your recommendation.

It seems that currently INTERNAL_REFERENCE is used for MIPS only, and not for
IA32 and X64. From the original commit 811cc11aab99d1fb9a662af5d023deb2eb7b19d2,
the INTERNAL_REFERENCE is used for "jump near, absolute indirect" for IA32,
i.e., the opcode is "FF /4". So when the code is moved by GC, the content of the
INTERNAL_REFERECE is +delta for the absolute address in the code.

For this CL, we want to use "jump near, relative", i.e., the opcode is "E9".
From reading the code from 811cc11aab99d1fb9a662af5d023deb2eb7b19d2, I realized
that this CL has an assumption that the deoptimization table entries will not
move. If it moves, I should use "+delta" in the relocation entry of the
optimized code, while if the optimized code (containing DeoptimizeIf) moves, I
have already used "-delta" in the code. If this assumption (the deoptimization
table entries are not moved by GC) right?

[danno, 2012-12-27 16:26:08]

Ah, you're right, although INTERNAL_REFERENCE is implemented in ia32, they are
intended only for absolute addresses within the same code object, so the
existing logic would move deopt entries in the wrong direction during
relocation.

Then if you really want to shrink the jump table, I really think deopt entries
need to be RUNTIME_ENTRIES, and as previous discussed all RUNTIME_ENTRIES need
to be 32-bits with trampolines to call through to C++ code in the non-deopt
entry case. Alternatively, you could support RUNTIME_ENTRIES in two modes,
depending on the opcode before the RUNTIME_ENTRY in the instruction steam,
treating the 32-bit and 64-bit pointers differently in RelocInfo::apply and
RelocInfo::Visit/VisitRuntimeEntry. This is more plumbing work to put an
abstraction around whether the RUNTIME_ENTRY is 32 or 64 bits (although
RelocInfo already seems to support a call called "target_address_size", so maybe
some of the abstraction is already there), but  think that's probably a more
efficient solution in the long run.

[haitao.feng, 2012-12-28 07:10:03]

danno, thanks for your comments.

I have uploaded a CL to use EXTERNAL_REFERENCE for the non-deopt entry case and
will update this CL to use RUNTIME_ENTRY. Now this CL depends on:
1. https://codereview.chromium.org/11566011/
2. https://codereview.chromium.org/11694008/

[haitao.feng, 2013-01-10 07:11:10]

I have updated this CL to use RUNTIME_ENTRY relocation entry which is
recommended by danno.

This CL depends on https://codereview.chromium.org/11825023/ which reserves the
deoptimization entries in the code range for X64.

[haitao.feng, 2013-01-30 04:44:09]

Updated this CL to the latest codes.

Now deoptimization entries for X64 are in the code range, so we could use direct
jmp/branch/call from optimized codes to deoptimization entries. We are using
RUNTIME_ENTRY relocation info for this purpose, so the RUNTIME_ENTRY usages on
deoptimization jump are the same for both IA32 and X64. Serializer/Deserializer
should work for X64 RUNTIME_ENTRY as for IA32.

[danno, 2013-02-06 15:00:38]

Thanks for the patch.

Before I look in much more detail, please take a look and address my high-level
concern about introducing platform-specific code in the common code-base.

https://codereview.chromium.org/11574027/diff/29001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11574027/diff/29001/src/objects.cc#newcode8627
src/objects.cc:8627: #if defined(V8_TARGET_ARCH_X64)
I would very much like to avoid having platform #ifdefs in shared code. I think
a better solution is to add a new method of RelocInfo, something like
set_runtime_entry_address, which has a platform-specific implementation and
makes the offset relative on platforms that support it, just like
set_target_address does. With that in place, you could always execute the code
below, getting the "raw" address using a specialized call similar to what code
targets do above, e.g.:

 Address p = it.rinfo()->absolute_runtime_entry_address(origin);
it.rinfo()->set_target_runtime_entry_address(origin);

[haitao.feng, 2013-02-07 05:08:05]

Thanks for your review.

I added two functions for each platform, "NeedsRewriteRuntimeEntry" and
"target_runtime_entry" to avoid platform #ifdef.

I am not quite familiar with ARM and MIPS instruction set, from the code I have
read, RUNTIME_ENTRY is only used for ARM and MIPS in the DeoptimizeIf and it
seems that the RUNTIME_ENTRY usage should be replaced by EXTERNAL_REFERENCE. If
so, RUNTIME_ENTRY are unused for those two platforms.

In this CL, target_runtime_entry is defined as target_address() for ARM and
MIPS. Maybe it should be defined as NULL if RUNTIME_ENTRY will be replaced by
EXTERNAL_REFERENCE.

https://codereview.chromium.org/11574027/diff/29001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11574027/diff/29001/src/objects.cc#newcode8627
src/objects.cc:8627: #if defined(V8_TARGET_ARCH_X64)
On 2013/02/06 15:00:38, danno wrote:
> I would very much like to avoid having platform #ifdefs in shared code. I
think
> a better solution is to add a new method of RelocInfo, something like
> set_runtime_entry_address, which has a platform-specific implementation and
> makes the offset relative on platforms that support it, just like
> set_target_address does. With that in place, you could always execute the code
> below, getting the "raw" address using a specialized call similar to what code
> targets do above, e.g.:
> 
>  Address p = it.rinfo()->absolute_runtime_entry_address(origin);
> it.rinfo()->set_target_runtime_entry_address(origin);
> 

Done.

[haitao.feng, 2013-02-08 03:54:52]

Updated this CL to use "target_runtime_entry" only to avoid platform #ifdef.

The correctness depends on https://codereview.chromium.org/12213080/ which
replaces the RUNTIME_ENTRY with EXTERNAL_REFERENCE for ARM and MIPS.

[haitao.feng, 2013-02-27 04:41:14]

Rebased with r13740. This is a self-contained patch and does not depend on the
other commits. 

I choose the name "NeedsInitializeRuntimeEntry" as the RuntimeEntry relocation
information has been initialized for IA32, ARM and MIPS when the generated codes
are copied while not for X64. For X64, it is handled by the similar way for
CodeTarget.

[danno, 2013-02-28 10:52:22]

Given recent developments, I am not sure this CL is a good idea at all.

Here's why. mstarzinger recently landed a change that accidentally broke the
DeoptimizeIf code on ia32, making all ia32 deopts go through the jump table.
Surprisingly, there was a performance improvement on Intel Core i5 of about 2%
for Octane. When we noticed the "problem" and "fixed" the code to produce the
direct jumps to the deoptimization table entries again, performance dropped on
Octane by 2%.

So it appears that using a jump table in all cases gives a significant
performance boost. Sven's guess is that it has something to do with branch
prediction and the distance of the branch target from the branch source, but
that's really just a guess.

In any case, unless you can demonstrate that this is not the case on x64 (i.e.
your change helps performance rather than hurts it), I don't think we should
pursue this patch.

[haitao.feng, 2013-02-28 11:44:54]

Thanks for the information.

This CL is still worth pursuing. I rebased this CL with r13740. In r13740 the
codes in DeOptimizeIf are nearly same for all the platforms. So I did not add 
"else if (!needs_lazy_deopt && frame_is_built_) {
-    // jump table entry for conditional deopts if possible.
-    ASSERT(cc != no_condition);
-    __ j(cc, entry, RelocInfo::RUNTIME_ENTRY); 
}" for X64. I have planned to update this CL to add this logic when I see
https://codereview.chromium.org/12340113 but have not done that yet. 

The value in this CL is that it unifies the code logic for CodeTarget and
RuntimeEntry for X64 and we could use "call" and "jmp" instead of "Call" and
"Jump" to save one instruction. Here is the performance data in my machine
(Intel(R) Core(TM) i7-2600).

../out/x64.release/d8 ./run.js

Before:
Richards: 18329
DeltaBlue: 23730
Crypto: 23478
RayTrace: 23019
EarleyBoyer: 36808
RegExp: 5565
Splay: 7000
NavierStokes: 24758
----
Score (version 7): 17385

After:
Richards: 18516
DeltaBlue: 23578
Crypto: 23580
RayTrace: 22866
EarleyBoyer: 37711
RegExp: 5576
Splay: 7262
NavierStokes: 25102
----
Score (version 7): 17555

[danno, 2013-03-07 14:57:22]

Thanks for the patch. Your general approach is fine, but there are few key
changes, see below.

https://codereview.chromium.org/11574027/diff/52001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/11574027/diff/52001/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:158: UNREACHABLE();
Please implement this for all platforms, see comments below.

https://codereview.chromium.org/11574027/diff/52001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11574027/diff/52001/src/objects.cc#newcode8627
src/objects.cc:8627: Address p = it.rinfo()->target_runtime_entry(origin);
Why do you need NeedsInitializeRuntimeEntry at all? If you make
target_runtime_entry always return the right thing (platform specific), then
this can be executed on all platforms. For platforms that don't change the
address because it is an absolute address, the set_target_address should do
nothing (it checks the current value and doesn't store if there is no change).

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64-inl.h
File src/x64/assembler-x64-inl.h (right):

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64-inl...
src/x64/assembler-x64-inl.h:89: void Assembler::emit_runtime_entry(Address
entry, RelocInfo::Mode rmode) {
I don't think you need to keep the runtime_entries_ table at all. Just subtract
the CodeRange base from entry, that gives you a 32-offset that you can store
directly in the runtime entry until it's copied into the code object.
runtime_entry_at on x64 return needs to return the offset + CodeRange.start.

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64.cc#...
src/x64/assembler-x64.cc:359: runtime_entries_(100),
Ad previously commented, I don't think you need this.

[haitao.feng, 2013-03-08 05:30:05]

Thanks for your review and recommendation. The codes are updated according to
your recommendation. Please take another look.

https://codereview.chromium.org/11574027/diff/52001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/11574027/diff/52001/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:158: UNREACHABLE();
On 2013/03/07 14:57:23, danno wrote:
> Please implement this for all platforms, see comments below.

Done.

https://codereview.chromium.org/11574027/diff/52001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11574027/diff/52001/src/objects.cc#newcode8627
src/objects.cc:8627: Address p = it.rinfo()->target_runtime_entry(origin);
On 2013/03/07 14:57:23, danno wrote:
> Why do you need NeedsInitializeRuntimeEntry at all? If you make
> target_runtime_entry always return the right thing (platform specific), then
> this can be executed on all platforms. For platforms that don't change the
> address because it is an absolute address, the set_target_address should do
> nothing (it checks the current value and doesn't store if there is no change).


Done.

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64-inl.h
File src/x64/assembler-x64-inl.h (right):

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64-inl...
src/x64/assembler-x64-inl.h:89: void Assembler::emit_runtime_entry(Address
entry, RelocInfo::Mode rmode) {
On 2013/03/07 14:57:23, danno wrote:
> I don't think you need to keep the runtime_entries_ table at all. Just
subtract
> the CodeRange base from entry, that gives you a 32-offset that you can store
> directly in the runtime entry until it's copied into the code object.
> runtime_entry_at on x64 return needs to return the offset + CodeRange.start. 

Done.

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/11574027/diff/52001/src/x64/assembler-x64.cc#...
src/x64/assembler-x64.cc:359: runtime_entries_(100),
On 2013/03/07 14:57:23, danno wrote:
> Ad previously commented, I don't think you need this.

Done.

[danno, 2013-03-08 09:16:30]

Thanks for the new patch, it's getting closer. Please see my comments below.

https://codereview.chromium.org/11574027/diff/72001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/11574027/diff/72001/src/assembler.h#newcode387
src/assembler.h:387: WriteBarrierMode mode = UPDATE_WRITE_BARRIER));
nit: please fix parameter indentation

https://codereview.chromium.org/11574027/diff/72001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11574027/diff/72001/src/objects.cc#newcode8816
src/objects.cc:8816: Address p = it.rinfo()->target_runtime_entry(origin,
delta);
I think you can, and should, avoid having the delta parameter. You don't use it
on x64 and ARM, and the only reason you need it is that when the assembler
outputs a RUNTIME_ENTRY into the code, it does uses an relative offset already.
If you ensure that on ia32 RUNTIME_ENTRIES are stored as absolute addresses in
the intermediate code buffer rather than relative addresses (i.e. rewrite call
and jmp in assembler-ia32.cc to not subtract the pc_ for rmode ==
RUNTIME_ENTRY), then CopyFrom doesn't need to apply any delta ever to
RUNTIME_ENTRIES.

https://codereview.chromium.org/11574027/diff/72001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/11574027/diff/72001/src/x64/assembler-x64.h#n...
src/x64/assembler-x64.h:564: inline Address runtime_entry_at(Address pc);
is this used any where? if not, please remove.

[haitao.feng, 2013-03-08 12:25:59]

Thanks for the review and recommendation. Please take another look.

https://codereview.chromium.org/11574027/diff/72001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/11574027/diff/72001/src/assembler.h#newcode387
src/assembler.h:387: WriteBarrierMode mode = UPDATE_WRITE_BARRIER));
On 2013/03/08 09:16:30, danno wrote:
> nit: please fix parameter indentation

Done.

https://codereview.chromium.org/11574027/diff/72001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11574027/diff/72001/src/objects.cc#newcode8816
src/objects.cc:8816: Address p = it.rinfo()->target_runtime_entry(origin,
delta);
On 2013/03/08 09:16:30, danno wrote:
> I think you can, and should, avoid having the delta parameter. You don't use
it
> on x64 and ARM, and the only reason you need it is that when the assembler
> outputs a RUNTIME_ENTRY into the code, it does uses an relative offset
already.
> If you ensure that on ia32 RUNTIME_ENTRIES are stored as absolute addresses in
> the intermediate code buffer rather than relative addresses (i.e. rewrite call
> and jmp in assembler-ia32.cc to not subtract the pc_ for rmode ==
> RUNTIME_ENTRY), then CopyFrom doesn't need to apply any delta ever to
> RUNTIME_ENTRIES.

Done.

https://codereview.chromium.org/11574027/diff/72001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/11574027/diff/72001/src/x64/assembler-x64.h#n...
src/x64/assembler-x64.h:564: inline Address runtime_entry_at(Address pc);
It is used by target_runtime_entry. 

On 2013/03/08 09:16:30, danno wrote:
> is this used any where? if not, please remove.

[danno, 2013-03-11 14:05:29]

lgtm, I'll land this for you. Thanks for the patch.

[danno, 2013-03-11 14:05:59]

Committed patchset #9 manually as r13901 (presubmit successful).

[danno, 2013-03-11 14:11:13]

Committed patchset #9 manually as r13903 (presubmit successful).