VM: New calling convention for generated code.

VM: New calling convention for generated code.

Instead of calling code object directly, call indirectly and
pass the code object in a register. The object pool is then loaded from
the code object. This is another preparation step for making generated code
relocatable.

All non-ia32 platforms:
No entry patching.

ARM:
PC marker (now code object) moves to the same place as on x64 (below saved PP, above saved FP).
R9 is now used as PP, R10 as CODE_REG.


BUG=
R=koda@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/1d343e5a7b75168fb6c9f86b64c55173cdbdc9b2

Committed: https://github.com/dart-lang/sdk/commit/bac82e2592238663c13dbb324a7dcb1da1a5b7ba

[Florian Schneider, 2015-06-18 13:41:43]

fschneider@google.com changed reviewers:
+ iposva@google.com, rmacnak@google.com, srdjan@google.com

[Florian Schneider, 2015-06-18 13:41:44]

This is the x64 only for now. It still needs to be ported to the other
platforms.

We need to figure out what to do on ia32: Keep the current calling convention or
adopt the same as on the other platforms. A lot of changes here are in
platform-independent files: object layout of Code, Instructions in
(raw_)object.h, deopt_instructions.cc, code_generator.cc, code_patcher.cc,
debugger.cc, stack_frame.cc etc.

Using the same convention on all platforms would simplify things a lot -
otherwise a lot of code needs to be made arch-specific.

[srdjan, 2015-06-18 21:29:47]

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:48:
Code::Handle(stub_code->UpdateStoreBufferCode()), kNotPatchable);
Code::Handle(isolate,

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:55: Code::Handle(stub_code->CallToRuntimeCode()),
kNotPatchable);
Code::Handle(isolate,

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:2646: Register pp) {
All arguments on one line

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/code_patcher_x64.cc
File runtime/vm/code_patcher_x64.cc (right):

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/code_patcher_x64...
runtime/vm/code_patcher_x64.cc:34: //   7: 41 ff 97 imm32  call [PP + off]
You may need to update this comment.

[Florian Schneider, 2015-06-29 14:50:24]

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:48:
Code::Handle(stub_code->UpdateStoreBufferCode()), kNotPatchable);
On 2015/06/18 21:29:46, srdjan wrote:
> Code::Handle(isolate, 

Done.

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:55: Code::Handle(stub_code->CallToRuntimeCode()),
kNotPatchable);
On 2015/06/18 21:29:46, srdjan wrote:
> Code::Handle(isolate,

Done.

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:2646: Register pp) {
On 2015/06/18 21:29:47, srdjan wrote:
> All arguments on one line

Done.

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/code_patcher_x64.cc
File runtime/vm/code_patcher_x64.cc (right):

https://codereview.chromium.org/1192103004/diff/1/runtime/vm/code_patcher_x64...
runtime/vm/code_patcher_x64.cc:34: //   7: 41 ff 97 imm32  call [PP + off]
On 2015/06/18 21:29:47, srdjan wrote:
> You may need to update this comment.

Done.

[rmacnak, 2015-09-09 01:37:48]

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/code...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/code...
runtime/vm/code_generator.cc:1442: // TODO(vegorov) figure out if it is GC safe.
The change to kFirstObjectSlotFromFp makes it safe, no?

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/disa...
File runtime/vm/disassembler_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/disa...
runtime/vm/disassembler_arm.cc:112: // formatting, except for register aliases
ctx (r9) and pp (r10).
except for register alias pp (r10).

[Florian Schneider, 2015-09-09 13:19:49]

Added first working MIPS version.

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/code...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/code...
runtime/vm/code_generator.cc:1442: // TODO(vegorov) figure out if it is GC safe.
On 2015/09/09 01:37:48, rmacnak wrote:
> The change to kFirstObjectSlotFromFp makes it safe, no?

I think so too. We do now visit the code object pointer in the stack frame.
Maybe it is not necessary, since code objects don't move for now,  but it can't
hurt in any case.

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/disa...
File runtime/vm/disassembler_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/120001/runtime/vm/disa...
runtime/vm/disassembler_arm.cc:112: // formatting, except for register aliases
ctx (r9) and pp (r10).
On 2015/09/09 01:37:48, rmacnak wrote:
> except for register alias pp (r10).

Done.

[rmacnak, 2015-09-11 00:52:46]

https://chromiumcodereview.appspot.com/1192103004/diff/180001/runtime/vm/stac...
File runtime/vm/stack_frame_arm.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/180001/runtime/vm/stac...
runtime/vm/stack_frame_arm.h:22: | PC marker         T|    (current frame's code
entry + offset)
current frame's code

[koda, 2015-09-11 13:22:04]

koda@google.com changed reviewers:
+ koda@google.com

[koda, 2015-09-11 13:22:05]

https://chromiumcodereview.appspot.com/1192103004/diff/180001/runtime/vm/stac...
File runtime/vm/stack_frame.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/180001/runtime/vm/stac...
runtime/vm/stack_frame.h:56: void set_pc_marker(uword value) {
Since this slot is traversed by GC, shouldn't this interface take a RawObject*
(or RawCode*)?

[Florian Schneider, 2015-09-11 14:30:23]

PTAL. ia32 port finished.

https://chromiumcodereview.appspot.com/1192103004/diff/180001/runtime/vm/stac...
File runtime/vm/stack_frame.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/180001/runtime/vm/stac...
runtime/vm/stack_frame.h:56: void set_pc_marker(uword value) {
On 2015/09/11 13:22:05, koda wrote:
> Since this slot is traversed by GC, shouldn't this interface take a RawObject*
> (or RawCode*)?

Done. (and yes, I'm not done with renaming pc-marker yet... for now everything
named pc-marker just contains a RawCode object)

[rmacnak, 2015-09-12 23:12:44]

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_arm.cc (left):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_arm.cc:3357: // Push 0 as saved PC for stub frames.
GC stack walker should be able to assert the pc-marker is never 0 now right?

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_arm.cc:1559: #ifdef DEBUG
This seems a bit heavy even for debug mode. When first landing this change, I
think it is okay, but we should consider putting it behind a flag after the
change has been checked in for a while.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_arm.cc:2727: object_pool_wrapper_.FindObject(target_code,
kPatchable));
patchable

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_ia32.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_ia32.cc:2626: LoadObject(CODE_REG, target);
Can't this just be call the entry point as before? On IA32 saving the PC-marker
is a push-immediate and there is no pool pointer to load, so there is no reason
to setup CODE_REG right?

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_ia32.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_ia32.h:1: // Copyright (c) 2014, the Dart project authors. 
Please see the AUTHORS file
Why?

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/code...
File runtime/vm/code_patcher.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/code...
runtime/vm/code_patcher.h:80: // TODO(fschneider): Rename if inly used for
lazy-deopt.
only

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/flow...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/flow...
runtime/vm/flow_graph_compiler_arm.cc:103: // Current PP, FP, and PC.
^PC marker

[Florian Schneider, 2015-09-14 11:15:15]

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_arm.cc (left):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_arm.cc:3357: // Push 0 as saved PC for stub frames.
On 2015/09/12 23:12:44, rmacnak wrote:
> GC stack walker should be able to assert the pc-marker is never 0 now right?

Done. Added assertion to StackFrame::GetCodeObject.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_arm.cc:1559: #ifdef DEBUG
On 2015/09/12 23:12:43, rmacnak wrote:
> This seems a bit heavy even for debug mode. When first landing this change, I
> think it is okay, but we should consider putting it behind a flag after the
> change has been checked in for a while.

Yes, it is, but my guess is that in DEBUG mode, generated code does not account
for the largest part of the runtime. I will check test-cycle times to confirm
this.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_arm.cc:2727: object_pool_wrapper_.FindObject(target_code,
kPatchable));
On 2015/09/12 23:12:44, rmacnak wrote:
> patchable

Done.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_ia32.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_ia32.cc:2626: LoadObject(CODE_REG, target);
On 2015/09/12 23:12:44, rmacnak wrote:
> Can't this just be call the entry point as before? On IA32 saving the
PC-marker
> is a push-immediate and there is no pool pointer to load, so there is no
reason
> to setup CODE_REG right?

Possibly - as it is now, patching the entry (CodePatcher::PatchEntry) is done by
just changing the instructions in the code object - therefore, the indirect
call.

We may be able to improve this - but I think it would require quite a bit of
additional ia32-specific code.

For now I wanted to avoid adding too much ia32-specific code. Let's check how
large the performance impact of this pattern is before deciding on the final
implementation.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
File runtime/vm/assembler_ia32.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/asse...
runtime/vm/assembler_ia32.h:1: // Copyright (c) 2014, the Dart project authors. 
Please see the AUTHORS file
On 2015/09/12 23:12:44, rmacnak wrote:
> Why?

By accident I guess.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/code...
File runtime/vm/code_patcher.h (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/code...
runtime/vm/code_patcher.h:80: // TODO(fschneider): Rename if inly used for
lazy-deopt.
On 2015/09/12 23:12:44, rmacnak wrote:
> only

Removed this line.

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/flow...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/200001/runtime/vm/flow...
runtime/vm/flow_graph_compiler_arm.cc:103: // Current PP, FP, and PC.
On 2015/09/12 23:12:44, rmacnak wrote:
> ^PC marker

Done. Removing redundant comments.

[rmacnak, 2015-09-15 23:05:42]

lgtm

https://chromiumcodereview.appspot.com/1192103004/diff/240001/runtime/vm/asse...
File runtime/vm/assembler_mips.h (left):

https://chromiumcodereview.appspot.com/1192103004/diff/240001/runtime/vm/asse...
runtime/vm/assembler_mips.h:919: void BranchLink(const ExternalLabel* label,
Patchability patchable);
Still used in unsimulated MIPS 

runtime/vm/stub_code_mips.cc:210:70: error: no matching function for call to
‘dart::Assembler::BranchLink(const dart::ExternalLabel*, dart::Patchability)’
   __ BranchLink(&NativeEntry::NativeCallWrapperLabel(), kNotPatchable);

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
runtime/vm/code_generator.cc:682: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
Should be able to push this into only IA32.

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
runtime/vm/code_generator.cc:1534: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
"

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
runtime/vm/code_generator.cc:1581: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
"

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/debu...
File runtime/vm/debugger_ia32.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/debu...
runtime/vm/debugger_ia32.cc:31: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
"

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/debu...
runtime/vm/debugger_ia32.cc:58: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
"

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/stub...
File runtime/vm/stub_code_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/stub...
runtime/vm/stub_code_arm.cc:453: __ ldr(IP, Address(FP, kCallerSpSlotFromFp *
kWordSize));
Add a comment that this special handling of CODE_REG is so we save the deopting
frame's value instead of the value used to call this stub.

[Florian Schneider, 2015-09-16 10:36:45]

https://chromiumcodereview.appspot.com/1192103004/diff/240001/runtime/vm/asse...
File runtime/vm/assembler_mips.h (left):

https://chromiumcodereview.appspot.com/1192103004/diff/240001/runtime/vm/asse...
runtime/vm/assembler_mips.h:919: void BranchLink(const ExternalLabel* label,
Patchability patchable);
On 2015/09/15 23:05:41, rmacnak wrote:
> Still used in unsimulated MIPS 
> 
> runtime/vm/stub_code_mips.cc:210:70: error: no matching function for call to
> ‘dart::Assembler::BranchLink(const dart::ExternalLabel*, dart::Patchability)’
>    __ BranchLink(&NativeEntry::NativeCallWrapperLabel(), kNotPatchable);

Done.

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
runtime/vm/code_generator.cc:682: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
On 2015/09/15 23:05:42, rmacnak wrote:
> Should be able to push this into only IA32.

Done.

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
runtime/vm/code_generator.cc:1534: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
On 2015/09/15 23:05:41, rmacnak wrote:
> "

Done.

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/code...
runtime/vm/code_generator.cc:1581: WritableInstructionsScope
writable(instrs.EntryPoint(), instrs.size());
On 2015/09/15 23:05:41, rmacnak wrote:
> "

Done.

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/stub...
File runtime/vm/stub_code_arm.cc (right):

https://chromiumcodereview.appspot.com/1192103004/diff/260001/runtime/vm/stub...
runtime/vm/stub_code_arm.cc:453: __ ldr(IP, Address(FP, kCallerSpSlotFromFp *
kWordSize));
On 2015/09/15 23:05:42, rmacnak wrote:
> Add a comment that this special handling of CODE_REG is so we save the
deopting
> frame's value instead of the value used to call this stub.

Done.

[Florian Schneider, 2015-09-16 11:06:04]

Committed patchset #16 (id:300001) manually as
1d343e5a7b75168fb6c9f86b64c55173cdbdc9b2.

[Florian Schneider, 2015-09-16 12:52:58]

On 2015/09/16 11:06:04, Florian Schneider wrote:
> Committed patchset #16 (id:300001) manually as
> 1d343e5a7b75168fb6c9f86b64c55173cdbdc9b2.

Had to revert because of Win64 test failures.

[rmacnak, 2015-09-17 01:04:40]

On 2015/09/16 12:52:58, Florian Schneider wrote:
> On 2015/09/16 11:06:04, Florian Schneider wrote:
> > Committed patchset #16 (id:300001) manually as
> > 1d343e5a7b75168fb6c9f86b64c55173cdbdc9b2.
> 
> Had to revert because of Win64 test failures.

Makes the instructions sections of --disassemble empty too :(

[Florian Schneider, 2015-09-17 09:07:00]

On 2015/09/17 01:04:40, rmacnak wrote:
> On 2015/09/16 12:52:58, Florian Schneider wrote:
> > On 2015/09/16 11:06:04, Florian Schneider wrote:
> > > Committed patchset #16 (id:300001) manually as
> > > 1d343e5a7b75168fb6c9f86b64c55173cdbdc9b2.
> > 
> > Had to revert because of Win64 test failures.
> 
> Makes the instructions sections of --disassemble empty too :(

On Windows?

[rmacnak, 2015-09-17 20:30:23]

On 2015/09/17 09:07:00, Florian Schneider wrote:
> On 2015/09/17 01:04:40, rmacnak wrote:
> > On 2015/09/16 12:52:58, Florian Schneider wrote:
> > > On 2015/09/16 11:06:04, Florian Schneider wrote:
> > > > Committed patchset #16 (id:300001) manually as
> > > > 1d343e5a7b75168fb6c9f86b64c55173cdbdc9b2.
> > > 
> > > Had to revert because of Win64 test failures.
> > 
> > Makes the instructions sections of --disassemble empty too :(
> 
> On Windows?

Linux. But nevermind it seems to be a general logging issue when precompiling.
The instructions are there but they are out of order with the rest of the code
metadata.

[Florian Schneider, 2015-09-18 18:43:41]

Ptal.

Uploaded new patchset fixing Windows issues: I fixed StackFrame::IsStubFrame and
made the profiler code more robust (and conservative since we don't have a PC
marker in the stack frame anymore). Win64 calling convention required fixing a
assembler cc-test as well.

Main affected files are

assembler_x64_test.cc
stack_frame.cc
profiler.cc

[rmacnak, 2015-09-18 20:04:28]

LGTM

[koda, 2015-09-18 20:14:34]

LGTM with minor grammar fix

https://codereview.chromium.org/1192103004/diff/340001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/1192103004/diff/340001/runtime/vm/stack_frame...
runtime/vm/stack_frame.cc:26: // Can't assert on Windows because this is called
from the profiler thread.
Comment somewhat difficult to parse (and "." should be ","), how about "On
Windows, the profiler calls this from a separate thread where Thread::Current()
is NULL, so we cannot create a NoSafepointScope."

[Florian Schneider, 2015-09-19 11:03:32]

https://codereview.chromium.org/1192103004/diff/340001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/1192103004/diff/340001/runtime/vm/stack_frame...
runtime/vm/stack_frame.cc:26: // Can't assert on Windows because this is called
from the profiler thread.
On 2015/09/18 20:14:34, koda wrote:
> Comment somewhat difficult to parse (and "." should be ","), how about "On
> Windows, the profiler calls this from a separate thread where
Thread::Current()
> is NULL, so we cannot create a NoSafepointScope."

Done.

[Florian Schneider, 2015-09-19 11:21:16]

Committed patchset #19 (id:360001) manually as
bac82e2592238663c13dbb324a7dcb1da1a5b7ba.