Uses an object pool on x64

Sets a register aside on x64 for use as a pool-pointer. It is loaded and restored from the code object on Frame entry and exit. All LoadObject calls that can, and many calls and jumps through ExternalLabels now use the pool-pointer. The --compiler-stats flag when running dart2js indicates that code size is reduced ~13%, and more is probably possible.

R=fschneider@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27295

[Ivan Posva, 2013-08-22 05:26:25]

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

https://codereview.chromium.org/22825023/diff/11001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2352: leaq(RSP, Address(RBP, -2 * kWordSize));  //
Restore stack pointer.
Wouldn't this be shorter?

if (restore_pp) {
  movq(PP, Address(RBP, -2 * kWordSize));
}
movq(RSP, RBP);
popq(RBP);

https://codereview.chromium.org/22825023/diff/11001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2452: movq(PP, Address(PP, 0));
popq(PP);
movq(PP, Address(PP, -object_pool_pc_dist));

https://codereview.chromium.org/22825023/diff/11001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2477: movq(PP, Address(PP, 0));
Wouldn't this be shorter?
movq(PP, Address(RSP, 1* kWordSize));
movq(PP, Address(PP, -object_pool_pc_dist - offset));

[Ivan Posva, 2013-09-03 17:31:32]

Florian,

Could you please take a look at this?

Thanks,
-Ivan

[Florian Schneider, 2013-09-04 09:39:47]

First round of comments.

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

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:173: LoadExternalLabel(label, kNotPatchable);
This assumes LoadExternalLabel stores into TMP. Why not specify the target
register explicitly as argument to LoadExternalLabel.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2260: if (Utils::IsInt(8, offset) && (patchable ==
kPatchable)) {
In the case that patching is allowed, why not generate the movq instruction with
an imm32 operand instead of adding nops?

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2266: void Assembler::LoadObject(Register dst, const
Object& object,
I find it too invasive to specify kPatchable/kNotPatchable at each call site.

There seem to be only few call-sites that need to be patchable and maybe those
should use a different function instead.

Or: How about using always imm32 for the constant-pool load? This would simplify
a lot. Code size would suffer a bit, but this can be optimized in a separate CL.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:657: bool CanLoadFromObjectPool(const Object&
object);
I don't see this function used anywhere outside the Assembler class: I'd make it
private.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:658: void LoadWordFromPoolOffset(Register dst,
Register pp, int32_t offset,
I don't see this function used anywhere outside the Assembler class: I'd make it
private.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:660: void LoadExternalLabel(const ExternalLabel*
label,
I don't see this function used anywhere outside the Assembler class: I'd make it
private and remove the optional arguments.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:661: Patchability = kPatchable,
Max one optional argument, and only if absolutely necessary.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:668: Patchability patchable = kPatchable, Register pp
= PP);
Max one optional argument, and only if absolutely necessary.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:701: void LeaveFrame(bool restore_pp = false);
This optional argument seems dangerous, too. Try to avoid it.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:775: Register new_pp = PP, Register new_pc =
kNoRegister);
Again, two optional arguments is not a good idea, especially if there are the
same type!

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:781: Register new_pp = PP, Register new_pc =
kNoRegister);
Again, two optional arguments is not a good idea, especially if there are the
same type!

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:796: void EnterStubFrame(bool save_pp = false);
This optional argument seems dangerous, too. Try to avoid it.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/debugger_x64.c...
runtime/vm/debugger_x64.cc:68: code[0] = 0x4c;  // movq r15,[rbp-0x10]
I know it was this way before, but wouldn't it be much nicer if we used our own
assembler to emit this code?

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/find_code_obje...
File runtime/vm/find_code_object_test.cc (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/find_code_obje...
runtime/vm/find_code_object_test.cc:17: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
Why change this?

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (left):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:141: 
Accidental change?

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:193: __
CallFromPool(&StubCode::Subtype1TestCacheLabel());
 Are there any direct calls to ExternalLabel left? If not, maybe it's better to
just rename it to Call?

[zra, 2013-09-04 21:00:40]

Thanks.

PTAL

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

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:173: LoadExternalLabel(label, kNotPatchable);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> This assumes LoadExternalLabel stores into TMP. Why not specify the target
> register explicitly as argument to LoadExternalLabel.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2260: if (Utils::IsInt(8, offset) && (patchable ==
kPatchable)) {
On 2013/09/04 09:39:47, Florian Schneider wrote:
> In the case that patching is allowed, why not generate the movq instruction
with
> an imm32 operand instead of adding nops?

I'm afraid I don't see a straightforward way of doing this. It seems messy to
me, but I could add a constructor to Address to force it to use the imm32 offset
encoding, though. Otherwise, since dst and pp can change, I'd be reproducing a
lot of Operand encoding code here, which seems bad.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2266: void Assembler::LoadObject(Register dst, const
Object& object,
On 2013/09/04 09:39:47, Florian Schneider wrote:
> I find it too invasive to specify kPatchable/kNotPatchable at each call site.
> 
> There seem to be only few call-sites that need to be patchable and maybe those
> should use a different function instead.
> 
> Or: How about using always imm32 for the constant-pool load? This would
simplify
> a lot. Code size would suffer a bit, but this can be optimized in a separate
CL.

I've used a different function. See above comment about using imm32 offset
everywhere.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:657: bool CanLoadFromObjectPool(const Object&
object);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> I don't see this function used anywhere outside the Assembler class: I'd make
it
> private.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:658: void LoadWordFromPoolOffset(Register dst,
Register pp, int32_t offset,
On 2013/09/04 09:39:47, Florian Schneider wrote:
> I don't see this function used anywhere outside the Assembler class: I'd make
it
> private.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:660: void LoadExternalLabel(const ExternalLabel*
label,
On 2013/09/04 09:39:47, Florian Schneider wrote:
> I don't see this function used anywhere outside the Assembler class: I'd make
it
> private and remove the optional arguments.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:661: Patchability = kPatchable,
On 2013/09/04 09:39:47, Florian Schneider wrote:
> Max one optional argument, and only if absolutely necessary.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:668: Patchability patchable = kPatchable, Register pp
= PP);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> Max one optional argument, and only if absolutely necessary.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:701: void LeaveFrame(bool restore_pp = false);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> This optional argument seems dangerous, too. Try to avoid it.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:775: Register new_pp = PP, Register new_pc =
kNoRegister);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> Again, two optional arguments is not a good idea, especially if there are the
> same type!

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:781: Register new_pp = PP, Register new_pc =
kNoRegister);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> Again, two optional arguments is not a good idea, especially if there are the
> same type!

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:796: void EnterStubFrame(bool save_pp = false);
On 2013/09/04 09:39:47, Florian Schneider wrote:
> This optional argument seems dangerous, too. Try to avoid it.

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/debugger_x64.c...
runtime/vm/debugger_x64.cc:68: code[0] = 0x4c;  // movq r15,[rbp-0x10]
On 2013/09/04 09:39:47, Florian Schneider wrote:
> I know it was this way before, but wouldn't it be much nicer if we used our
own
> assembler to emit this code?

Yes. Perhaps for another CL.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/find_code_obje...
File runtime/vm/find_code_object_test.cc (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/find_code_obje...
runtime/vm/find_code_object_test.cc:17: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
On 2013/09/04 09:39:47, Florian Schneider wrote:
> Why change this?

With the smaller code size, 25000 iterations is no longer enough to generate
more than 1MB of code.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (left):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:141: 
On 2013/09/04 09:39:47, Florian Schneider wrote:
> Accidental change?

Done.

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:193: __
CallFromPool(&StubCode::Subtype1TestCacheLabel());
On 2013/09/04 09:39:47, Florian Schneider wrote:
>  Are there any direct calls to ExternalLabel left? If not, maybe it's better
to
> just rename it to Call?

Yes, there is at least the call in RuntimeEntry::Call().

[srdjan, 2013-09-04 22:57:22]

For next CL suggesting to use fixed addresses for null, true and false, instead
of loading via xxxPool assembly instructions

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

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2260: if (Utils::IsInt(8, offset) && (patchable ==
kPatchable)) {
On 2013/09/04 21:00:41, zra wrote:
> On 2013/09/04 09:39:47, Florian Schneider wrote:
> > In the case that patching is allowed, why not generate the movq instruction
> with
> > an imm32 operand instead of adding nops?
> 
> I'm afraid I don't see a straightforward way of doing this. It seems messy to
> me, but I could add a constructor to Address to force it to use the imm32
offset
> encoding, though. Otherwise, since dst and pp can change, I'd be reproducing a
> lot of Operand encoding code here, which seems bad.

I am with Florian here, it would simplify the code patcher quite a bit. What you
would need is a new/changed Address constructor, isn't it?

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

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:175: {
Extra block/scope unnecessary?

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2095: {
ditto and elsewhere?

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2199: int32_t Assembler::AddObject(const Object&
obj) {
Why int32_t instead of intptr_t? Length() is intptr_t.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2204: if (object_pool_.IsNull()) {
Should this be an assert? Assembler constructor always allocates an object_pool_
if we are not in VM isolate.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2208: if (object_pool_.At(i) == obj.raw()) {
This is slow for large code objects, i.e., compilation time will become
considerably slower for very large functions. This is most likely the reason for
the timeout that you are seeing in the FindCode test.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2218: Patchability patchable) {
s/int32_t/intptr_t/ ?

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2222: object_pool_ =
GrowableObjectArray::New(Heap::kOld);
Ditto

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2224: const word address = label->address();
const uword

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2227: const Smi& smi = Smi::Handle(Smi::New(address
>> kSmiTagShift));
const Smi& smi = Smi::Handle(reinterpret_cast<RawSmi*>(address)) ?

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2261: // have to pad with nops.
Why not generate a movq that always emits long form?

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:845: Patchability,
Give the argument its nam, e.g., patchable.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/find_code_obje...
File runtime/vm/find_code_object_test.cc (right):

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/find_code_obje...
runtime/vm/find_code_object_test.cc:17: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
Maybe you could have an X64 specific loop count that does no time out but still
generates enough code for this test?

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:169: __ CompareObject(bool_register,
Object::Handle(Object::null()));
Object::Handle() is same as Object::Handle(Object::null()), but is shorter (also
below).

[zra, 2013-09-05 00:23:11]

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

https://codereview.chromium.org/22825023/diff/68001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2260: if (Utils::IsInt(8, offset) && (patchable ==
kPatchable)) {
On 2013/09/04 22:57:23, srdjan wrote:
> On 2013/09/04 21:00:41, zra wrote:
> > On 2013/09/04 09:39:47, Florian Schneider wrote:
> > > In the case that patching is allowed, why not generate the movq
instruction
> > with
> > > an imm32 operand instead of adding nops?
> > 
> > I'm afraid I don't see a straightforward way of doing this. It seems messy
to
> > me, but I could add a constructor to Address to force it to use the imm32
> offset
> > encoding, though. Otherwise, since dst and pp can change, I'd be reproducing
a
> > lot of Operand encoding code here, which seems bad.
> 
> I am with Florian here, it would simplify the code patcher quite a bit. What
you
> would need is a new/changed Address constructor, isn't it?

Added a constructor to Address to generate a fixed-size offset.

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

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:175: {
On 2013/09/04 22:57:23, srdjan wrote:
> Extra block/scope unnecessary?

It's needed for EnsureCapacity.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2095: {
On 2013/09/04 22:57:23, srdjan wrote:
> ditto and elsewhere?

I've tried to remove, but EnsureCapacity doesn't like it when LoadExternalLabel
is in the same scope with it.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2199: int32_t Assembler::AddObject(const Object&
obj) {
On 2013/09/04 22:57:23, srdjan wrote:
> Why int32_t instead of intptr_t? Length() is intptr_t.

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2204: if (object_pool_.IsNull()) {
On 2013/09/04 22:57:23, srdjan wrote:
> Should this be an assert? Assembler constructor always allocates an
object_pool_
> if we are not in VM isolate.

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2208: if (object_pool_.At(i) == obj.raw()) {
On 2013/09/04 22:57:23, srdjan wrote:
> This is slow for large code objects, i.e., compilation time will become
> considerably slower for very large functions. This is most likely the reason
for
> the timeout that you are seeing in the FindCode test.

Will add a hashmap from obj.raw() to object pool indexes in a future CL.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2218: Patchability patchable) {
On 2013/09/04 22:57:23, srdjan wrote:
> s/int32_t/intptr_t/ ?

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2222: object_pool_ =
GrowableObjectArray::New(Heap::kOld);
On 2013/09/04 22:57:23, srdjan wrote:
> Ditto

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2224: const word address = label->address();
On 2013/09/04 22:57:23, srdjan wrote:
> const uword

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2227: const Smi& smi = Smi::Handle(Smi::New(address
>> kSmiTagShift));
On 2013/09/04 22:57:23, srdjan wrote:
> const Smi& smi = Smi::Handle(reinterpret_cast<RawSmi*>(address)) ?

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2261: // have to pad with nops.
On 2013/09/04 22:57:23, srdjan wrote:
> Why not generate a movq that always emits long form?

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:845: Patchability,
On 2013/09/04 22:57:23, srdjan wrote:
> Give the argument its nam, e.g., patchable.

Done.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/find_code_obje...
File runtime/vm/find_code_object_test.cc (right):

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/find_code_obje...
runtime/vm/find_code_object_test.cc:17: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_X64)
On 2013/09/04 22:57:23, srdjan wrote:
> Maybe you could have an X64 specific loop count that does no time out but
still
> generates enough code for this test?

It times out even when the loop count is too small to generate enough code.

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/22825023/diff/83001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:169: __ CompareObject(bool_register,
Object::Handle(Object::null()));
On 2013/09/04 22:57:23, srdjan wrote:
> Object::Handle() is same as Object::Handle(Object::null()), but is shorter
(also
> below).

Done.

[Florian Schneider, 2013-09-05 10:57:45]

Looking already much better! Here is another round of comments: Mostly I tried
to make the code simpler and concise.

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

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:170: void Assembler::CallFromPool(const
ExternalLabel* label) {
I'd still rename this to Call to be consistent with the other architectures that
also use a constant pool. You could pass the pool pointer as a parameter to make
it clearer, like it's done for Jump.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2033: void
Assembler::ConditionalJumpFromPool(Condition condition,
How about renaming this to just J?

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2107: void Assembler::JumpFromPool(const
ExternalLabel* label, Register pp) {
Likewise rename to just Jmp. It's clear from the signature that the constant
pool is used because you pass the pool pointer as a parameter.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2199: intptr_t Assembler::AddObject(const Object&
obj) {
AddObject is confusing because it does not always add an object. better:
FindObject, GetObject, GetConstant?

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2205: // TODO(zra): This can be slow. Add a hash map
from obj.raw() to
I think it's ok to land this TODO, but it has to be addressed right away after
landing: Look at the implementation of the symbol table, or the canonicalized
type arguments (object.cc:3507)

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2230: // TODO(zra): This can be slow. Add a hash map
from obj.raw() to
I think it's ok to land this TODO, but it has to be addressed right away after
landing.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2569: Register new_pp, Register new_pc) {
If new_pc == kNoRegister, then new_pp is not used at all. There are a number of
sites where you call

EnterDartFrame(x, PP, kNoRegister)

This should rather be

EnterDartFrame(x, kNoRegister, kNoRegister) is no good either.

I think this should be different functions with a well-describing name.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2590: movq(PP, Address(PP, -object_pool_pc_dist -
offset));
This PC-relative addressing procedure is complicated. The constant pool of this
Assember object is at a known address. Couldn't you just load it as a normal
imm64?

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:186: Address(Register base, int32_t disp, bool fixed)
{
Please add a test to assembler_x64_test.cc for this encodings.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:186: Address(Register base, int32_t disp, bool fixed)
{
Consider making this private and add Assembler as a friend class of Address. I
think this should normally not be used by other classes.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:663: void LoadObjectFromPool(Register dst, const
Object& obj, Register pp);
You really only need 

LoadObject(Register dst, const Object& obj, Register pp);

This way, you also catch all remaining call-sites of LoadObject via compile-time
errors and can inspect them one-by-one. If all sites have PP as pool register,
then you could even remove the pp parameter.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:853: Patchability patchabe,
s/patchabe/patchable/

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
File runtime/vm/instructions_x64.cc (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
runtime/vm/instructions_x64.cc:15: int32_t offset =
*reinterpret_cast<int32_t*>(start + 1);
Why +1 here? Instead you can just adjust every call-site.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
runtime/vm/instructions_x64.cc:17: return (offset -
Array::data_offset())/kWordSize;
Spaces around /

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
File runtime/vm/instructions_x64.h (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
runtime/vm/instructions_x64.h:41: static int IndexFromPPLoad(uword start);
s/int/intptr_t/

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:102: // Generate 4 bytes of NOPs so that
the debugger can patch the
How is 4 computed? The number of nops should depend on the instructions that the
debugger patches here (pattern length). Please add an assert that the number of
nops is correct.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:314: __ LoadObjectFromPool(result,
value(), PP);
This should just be

LoadObject(result, value())


because LoadObject is implemented as follows:

void LoadObject(Register dst, const Object& object) {
  LoadObjectFromPool(dst, object, PP);
}

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:509: __ LoadObjectFromPool(RAX,
Bool::Get(kind != Token::kEQ), PP);
If you require an extra parameter there is no need to rename the function
itself.

s/LoadObjectFromPool/LoadObject/

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:522: __ LoadObject(ic_data_reg,
equality_ic_data);
What about this LoadObject?

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/stub_code_x64....
runtime/vm/stub_code_x64.cc:388: __ EnterFrame(0);
It is very confusing to start with EnterFrame here and leave with
LeaveFrameWithPP

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/stub_code_x64....
runtime/vm/stub_code_x64.cc:429: __ EnterFrame(0);
It is very confusing to start with EnterFrame here and leave with
LeaveFrameWithPP

[zra, 2013-09-05 20:29:25]

Forgot to mention earlier that going with a fixed-size imm32 offset on the load
from the object pool results in the space savings being reduced from ~13% to
~9%, or an increase of ~320KB on dart2js.

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

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:170: void Assembler::CallFromPool(const
ExternalLabel* label) {
On 2013/09/05 10:57:45, Florian Schneider wrote:
> I'd still rename this to Call to be consistent with the other architectures
that
> also use a constant pool. You could pass the pool pointer as a parameter to
make
> it clearer, like it's done for Jump.

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2033: void
Assembler::ConditionalJumpFromPool(Condition condition,
On 2013/09/05 10:57:45, Florian Schneider wrote:
> How about renaming this to just J?

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2107: void Assembler::JumpFromPool(const
ExternalLabel* label, Register pp) {
On 2013/09/05 10:57:45, Florian Schneider wrote:
> Likewise rename to just Jmp. It's clear from the signature that the constant
> pool is used because you pass the pool pointer as a parameter.

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2199: intptr_t Assembler::AddObject(const Object&
obj) {
On 2013/09/05 10:57:45, Florian Schneider wrote:
> AddObject is confusing because it does not always add an object. better:
> FindObject, GetObject, GetConstant?

Changed to FindObject. Likewise, changed AddExternalLabel to FindExternalLabel.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2205: // TODO(zra): This can be slow. Add a hash map
from obj.raw() to
On 2013/09/05 10:57:45, Florian Schneider wrote:
> I think it's ok to land this TODO, but it has to be addressed right away after
> landing: Look at the implementation of the symbol table, or the canonicalized
> type arguments (object.cc:3507)

Acknowledged.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2230: // TODO(zra): This can be slow. Add a hash map
from obj.raw() to
On 2013/09/05 10:57:45, Florian Schneider wrote:
> I think it's ok to land this TODO, but it has to be addressed right away after
> landing.

Acknowledged.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2569: Register new_pp, Register new_pc) {
On 2013/09/05 10:57:45, Florian Schneider wrote:
> If new_pc == kNoRegister, then new_pp is not used at all. There are a number
of
> sites where you call
> 
> EnterDartFrame(x, PP, kNoRegister)
> 
> This should rather be
> 
> EnterDartFrame(x, kNoRegister, kNoRegister) is no good either.
> 
> I think this should be different functions with a well-describing name.
> 

Created and used function EnterDartFrameWithInfo

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2590: movq(PP, Address(PP, -object_pool_pc_dist -
offset));
On 2013/09/05 10:57:45, Florian Schneider wrote:
> This PC-relative addressing procedure is complicated. The constant pool of
this
> Assember object is at a known address. Couldn't you just load it as a normal
> imm64? 

After discussion with Ivan and Srdjan, they recommended evaluating this change
in a separate CL.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:186: Address(Register base, int32_t disp, bool fixed)
{
On 2013/09/05 10:57:45, Florian Schneider wrote:
> Consider making this private and add Assembler as a friend class of Address. I
> think this should normally not be used by other classes.

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:186: Address(Register base, int32_t disp, bool fixed)
{
On 2013/09/05 10:57:45, Florian Schneider wrote:
> Please add a test to assembler_x64_test.cc for this encodings.

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:663: void LoadObjectFromPool(Register dst, const
Object& obj, Register pp);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> You really only need 
> 
> LoadObject(Register dst, const Object& obj, Register pp);
> 
> This way, you also catch all remaining call-sites of LoadObject via
compile-time
> errors and can inspect them one-by-one. If all sites have PP as pool register,
> then you could even remove the pp parameter.

LoadObject is used in arch generic code. Changing its signature will require
adding an argument at all call-sites across all architectures. I opted to just
use LoadObject everwhere the pool-pointer is PP, and LoadObjectFromPool where
the pool-pointer isn't PP.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:853: Patchability patchabe,
On 2013/09/05 10:57:45, Florian Schneider wrote:
> s/patchabe/patchable/

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
File runtime/vm/instructions_x64.cc (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
runtime/vm/instructions_x64.cc:15: int32_t offset =
*reinterpret_cast<int32_t*>(start + 1);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> Why +1 here? Instead you can just adjust every call-site.

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
runtime/vm/instructions_x64.cc:17: return (offset -
Array::data_offset())/kWordSize;
On 2013/09/05 10:57:45, Florian Schneider wrote:
> Spaces around /

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
File runtime/vm/instructions_x64.h (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/instructions_x...
runtime/vm/instructions_x64.h:41: static int IndexFromPPLoad(uword start);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> s/int/intptr_t/

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:102: // Generate 4 bytes of NOPs so that
the debugger can patch the
On 2013/09/05 10:57:45, Florian Schneider wrote:
> How is 4 computed? The number of nops should depend on the instructions that
the
> debugger patches here (pattern length). Please add an assert that the number
of
> nops is correct.

Moved this sequence into the Assembler function ReturnPatchable so that I could
write an assert on its length.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:314: __ LoadObjectFromPool(result,
value(), PP);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> This should just be
> 
> LoadObject(result, value())
> 
> 
> because LoadObject is implemented as follows:
> 
> void LoadObject(Register dst, const Object& object) {
>   LoadObjectFromPool(dst, object, PP);
> }

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:509: __ LoadObjectFromPool(RAX,
Bool::Get(kind != Token::kEQ), PP);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> If you require an extra parameter there is no need to rename the function
> itself.
> 
> s/LoadObjectFromPool/LoadObject/

Using LoadObject everywhere the pool pointer is PP since there is no longer a
distinction between patchable and non-patchable LoadObject.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:522: __ LoadObject(ic_data_reg,
equality_ic_data);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> What about this LoadObject?

Done.

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/stub_code_x64....
runtime/vm/stub_code_x64.cc:388: __ EnterFrame(0);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> It is very confusing to start with EnterFrame here and leave with
> LeaveFrameWithPP

Inlined LeaveFrameWithPP (which ends with LeaveFrame).

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/stub_code_x64....
runtime/vm/stub_code_x64.cc:429: __ EnterFrame(0);
On 2013/09/05 10:57:45, Florian Schneider wrote:
> It is very confusing to start with EnterFrame here and leave with
> LeaveFrameWithPP

Inlined LeaveFrameWithPP (which ends with LeaveFrame).

[Florian Schneider, 2013-09-06 09:58:15]

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:663: void LoadObjectFromPool(Register dst, const
Object& obj, Register pp);
On 2013/09/05 20:29:26, zra wrote:
> On 2013/09/05 10:57:45, Florian Schneider wrote:
> > You really only need 
> > 
> > LoadObject(Register dst, const Object& obj, Register pp);
> > 
> > This way, you also catch all remaining call-sites of LoadObject via
> compile-time
> > errors and can inspect them one-by-one. If all sites have PP as pool
register,
> > then you could even remove the pp parameter.
> 
> LoadObject is used in arch generic code. Changing its signature will require
> adding an argument at all call-sites across all architectures. I opted to just
> use LoadObject everwhere the pool-pointer is PP, and LoadObjectFromPool where
> the pool-pointer isn't PP. 

Ok. But here is only one arch-independent place in
FlowGraphCompiler::EmitUnoptimizedStaticCall where LoadObject is used. You could
just move this function to the arch-specific files.

https://codereview.chromium.org/22825023/diff/110001/runtime/tests/vm/vm.status
File runtime/tests/vm/vm.status (right):

https://codereview.chromium.org/22825023/diff/110001/runtime/tests/vm/vm.stat...
runtime/tests/vm/vm.status:54: cc/FindCodeObject: Timeout
Issue number is missing.

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

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:147: // Encode call(TMP).
This should just be: 

__ call(TMP)

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:158: {
call(TMP)

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:175: {
call(TMP)

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2096: // Encode jmp(TMP).
jmp(TMP)

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2108: {
jmp(TMP)

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2228: // calls.
I find the whole approach a little brittle: Forgetting to emit a patchable call
at sites that will be patched can lead to bugs. I'd like an assertion that a
unpatchable site is never accidentally patched. Maybe mark the entries in the
constant pool as shared and assert in debug mode that shared entries are
immutable?

Or else: How about patching the index into the constant pool instead of changing
pool entries? This would be very similar to the way patching code was done
before, just instead of replacing the imm64 address, replacing an imm32 pool
index.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.h:832: void FixedSizeAddressOffsetTest();
I see - Let me suggest to add a static factory method to Address:

static Address AddressBaseImm32(Register base, int32_t disp);

This way you can add the test function to the _test file - and the name states
better what it generates than the overloaded constructor with an extra bool
argument.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:67: ASSERT((code[0] == 0x49) && (code[1] == 0xbb));
Please use the assembler here. Something along these lines should work:

MemoryRegion code(reinterpret_cast<void*>(pc_ - 13), 13);
Assembler asm();

asm.movq(...);
asm.movq(...);
asm.ret();
asm.nop(4);

asm.FinalizeInstructions(code);

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:77: code[9] = 0x0F;
Is this part of the nop?

[srdjan, 2013-09-06 15:55:51]

LGTM after addressing Florian's comments.

[srdjan, 2013-09-06 16:22:44]

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

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2217: Patchability patchable) {
fix indent

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2235: }
Could you replace this code with call to Assembler::FindObject?

[zra, 2013-09-06 17:53:25]

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/99001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:663: void LoadObjectFromPool(Register dst, const
Object& obj, Register pp);
On 2013/09/06 09:58:15, Florian Schneider wrote:
> On 2013/09/05 20:29:26, zra wrote:
> > On 2013/09/05 10:57:45, Florian Schneider wrote:
> > > You really only need 
> > > 
> > > LoadObject(Register dst, const Object& obj, Register pp);
> > > 
> > > This way, you also catch all remaining call-sites of LoadObject via
> > compile-time
> > > errors and can inspect them one-by-one. If all sites have PP as pool
> register,
> > > then you could even remove the pp parameter.
> > 
> > LoadObject is used in arch generic code. Changing its signature will require
> > adding an argument at all call-sites across all architectures. I opted to
just
> > use LoadObject everwhere the pool-pointer is PP, and LoadObjectFromPool
where
> > the pool-pointer isn't PP. 
> 
> Ok. But here is only one arch-independent place in
> FlowGraphCompiler::EmitUnoptimizedStaticCall where LoadObject is used. You
could
> just move this function to the arch-specific files.

Done with pool pointer argument added.

https://codereview.chromium.org/22825023/diff/110001/runtime/tests/vm/vm.status
File runtime/tests/vm/vm.status (right):

https://codereview.chromium.org/22825023/diff/110001/runtime/tests/vm/vm.stat...
runtime/tests/vm/vm.status:54: cc/FindCodeObject: Timeout
On 2013/09/06 09:58:15, Florian Schneider wrote:
> Issue number is missing.

Done.

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

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:147: // Encode call(TMP).
On 2013/09/06 09:58:15, Florian Schneider wrote:
> This should just be: 
> 
> __ call(TMP) 

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:158: {
On 2013/09/06 09:58:15, Florian Schneider wrote:
> call(TMP)

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:175: {
On 2013/09/06 09:58:15, Florian Schneider wrote:
> call(TMP)

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2096: // Encode jmp(TMP).
On 2013/09/06 09:58:15, Florian Schneider wrote:
> jmp(TMP)

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2108: {
On 2013/09/06 09:58:15, Florian Schneider wrote:
> jmp(TMP)

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2217: Patchability patchable) {
On 2013/09/06 16:22:44, srdjan wrote:
> fix indent

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2228: // calls.
On 2013/09/06 09:58:15, Florian Schneider wrote:
> I find the whole approach a little brittle: Forgetting to emit a patchable
call
> at sites that will be patched can lead to bugs. I'd like an assertion that a
> unpatchable site is never accidentally patched. Maybe mark the entries in the
> constant pool as shared and assert in debug mode that shared entries are
> immutable?
> 
> Or else: How about patching the index into the constant pool instead of
changing
> pool entries? This would be very similar to the way patching code was done
> before, just instead of replacing the imm64 address, replacing an imm32 pool
> index.

It is equivalent to check that a patchable external label is never re-used by a
non-patchable external label. So I've added a GrowableArray<Patchability> to
Assembler, and it has the state of the object_pool_ entries. Before reusing an
entry for a non-patchable external label, I check that the entry isn't
patchable. Srdjan thinks it would be too expensive to copy the bookkeeping over
to the Code object to check at runtime.

We're trying to remove places where we write code, so I don't think we should
patch the index instead of altering the object pool entry.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2235: }
On 2013/09/06 16:22:44, srdjan wrote:
> Could you replace this code with call to Assembler::FindObject?

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.h:832: void FixedSizeAddressOffsetTest();
On 2013/09/06 09:58:15, Florian Schneider wrote:
> I see - Let me suggest to add a static factory method to Address:
> 
> static Address AddressBaseImm32(Register base, int32_t disp);
> 
> This way you can add the test function to the _test file - and the name states
> better what it generates than the overloaded constructor with an extra bool
> argument.

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:67: ASSERT((code[0] == 0x49) && (code[1] == 0xbb));
On 2013/09/06 09:58:15, Florian Schneider wrote:
> Please use the assembler here. Something along these lines should work:
> 
> MemoryRegion code(reinterpret_cast<void*>(pc_ - 13), 13);
> Assembler asm();
> 
> asm.movq(...);
> asm.movq(...);
> asm.ret();
> asm.nop(4);
> 
> asm.FinalizeInstructions(code);
> 

Done.

https://codereview.chromium.org/22825023/diff/110001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:77: code[9] = 0x0F;
On 2013/09/06 09:58:15, Florian Schneider wrote:
> Is this part of the nop?

Replaced with Assembler use.

[Florian Schneider, 2013-09-09 09:55:59]

LGTM.

https://codereview.chromium.org/22825023/diff/141001/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

https://codereview.chromium.org/22825023/diff/141001/runtime/vm/debugger_x64....
runtime/vm/debugger_x64.cc:70: MemoryRegion
code_region(reinterpret_cast<void*>(pc_ - 13), 13);
MemoryRegion code_region(reinterpret_cast<void*>(code), 13);

And thanks for fixing this! It looks much cleaner.

[zra, 2013-09-09 15:38:25]

Thanks!

[zra, 2013-09-09 15:39:41]

Committed patchset #14 manually as r27295 (presubmit successful).