Make x64 to use the RecordWrite stub. This is a step towards getting

Make x64 to use the RecordWrite stub.  This is a step towards getting
incremental GC working on x64.
Committed: http://code.google.com/p/v8/source/detail?r=8221

[Vyacheslav Egorov (Chromium), 2011-06-07 14:01:27]

LGTM

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:197: // Update the write barrier.  This clobbers rax
and rbx.
rogue space before This

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3151: __ movq(object, elements);
Is this still required?

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3152: __ RememberedSetHelper(
I think ia32 has a comment that explains why we don't need to inform incremental
marker here.

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

http://codereview.chromium.org/7122003/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2237: }
Missing WriteBarrier (just for the record).

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:344: FLAG_incremental_marking == false) {
== false is just too strange if we have the same in ia32 we should fix it.

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:3741: bool Aliasing(Register r1, Register r2,
Register r3, Register r4) {
Aliasing is a very strange name.

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:3793: j(cc, condition_met, condition_met_near);
why conditions_met_near is called *_near not *_distance?

[Lasse Reichstein, 2011-06-07 14:02:53]

Partial comments only.

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) {
Rename to get_byte/set_byte. Or read_byte/patch_byte.
There is nothing opcode-specific in this.

http://codereview.chromium.org/7122003/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:5189: __ jmp(&skip_non_incremental_part,
Label::kNear);
But why overwrite?
Why not just:
  if (!HEAP->incremental_marking()->IsMarking()) {
    __ nop(2);
  } else {
    __ jmp(&....);
  }
And is this even safe? The label is unbound, so won't we be overwriting this
later?

http://codereview.chromium.org/7122003/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:5190: if (!HEAP->incremental_marking()->IsMarking()) {
HEAP should be replaced by something faster. Maybe you can get the isolate from
the MacroAssembler.

[Erik Corry, 2011-06-08 10:38:55]

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) {
On 2011/06/07 14:02:53, Lasse Reichstein wrote:
> Rename to get_byte/set_byte. Or read_byte/patch_byte.
> There is nothing opcode-specific in this.

No, but it makes the call site clearer.

http://codereview.chromium.org/7122003/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:5189: __ jmp(&skip_non_incremental_part,
Label::kNear);
On 2011/06/07 14:02:53, Lasse Reichstein wrote:
> But why overwrite?
> Why not just:
>   if (!HEAP->incremental_marking()->IsMarking()) {
>     __ nop(2);
>   } else {
>     __ jmp(&....);
>   }
> And is this even safe? The label is unbound, so won't we be overwriting this
> later?

We will be ovewriting the opcode every time we start and end an incremental
mark-sweep collection.  The overwrite is only of the offset, not the opcode.

http://codereview.chromium.org/7122003/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:5190: if (!HEAP->incremental_marking()->IsMarking()) {
On 2011/06/07 14:02:53, Lasse Reichstein wrote:
> HEAP should be replaced by something faster. Maybe you can get the isolate
from
> the MacroAssembler.

Done.

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:197: // Update the write barrier.  This clobbers rax
and rbx.
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> rogue space before This

It's a fixed width font:  http://en.wikipedia.org/wiki/Sentence_spacing

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3151: __ movq(object, elements);
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> Is this still required?

No, good point.

http://codereview.chromium.org/7122003/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3152: __ RememberedSetHelper(
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> I think ia32 has a comment that explains why we don't need to inform
incremental
> marker here.

Done.

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

http://codereview.chromium.org/7122003/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2237: }
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> Missing WriteBarrier (just for the record).

Thanks.

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:344: FLAG_incremental_marking == false) {
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> == false is just too strange if we have the same in ia32 we should fix it.

Done.

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:3741: bool Aliasing(Register r1, Register r2,
Register r3, Register r4) {
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> Aliasing is a very strange name.

AreAliased

http://codereview.chromium.org/7122003/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:3793: j(cc, condition_met, condition_met_near);
On 2011/06/07 14:01:27, Vyacheslav Egorov wrote:
> why conditions_met_near is called *_near not *_distance?

Done.

[Lasse Reichstein, 2011-06-08 10:58:45]

LGTM

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) {
I don't see us using get_opcode anywhere. Just remove it.

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) {
The call site can make its own local functions by any name, which call the
generic method on the assembler.

The generic method is there now, and public, for anyone to use at any time.
For that reason alone, I think it should be less bound to the current use-case.
E.g., I think it should take a zero-based index, not a pc-based index. It's easy
to convert between the two (using pc_offset()).

Or, preferably, replace it with a macro like:
 PatchableNearJump(byte opcode, Label* label) {
   db(opcode);
   emit_near_disp(label);
 }

but there is a visibility issue to work around with that, since MacroAssembler
cant see emit_near_disp. Perhaps make a db_near_disp(Label*) method for that.

http://codereview.chromium.org/7122003/diff/3010/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7122003/diff/3010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:5193: // branch when we start and stop incremental
heap marking.
LGTM

http://codereview.chromium.org/7122003/diff/3010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:5197: masm->set_opcode(-2, kTwoByteNopInstruction);
Still think "set_opcode" is too specific for a byte-patcher.

[Vyacheslav Egorov (Chromium), 2011-06-08 11:02:19]

still LGTM

[Erik Corry, 2011-06-08 11:27:11]

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) {
On 2011/06/08 10:58:46, Lasse Reichstein wrote:
> The call site can make its own local functions by any name, which call the
> generic method on the assembler.
> 
> The generic method is there now, and public, for anyone to use at any time.
> For that reason alone, I think it should be less bound to the current
use-case.
> E.g., I think it should take a zero-based index, not a pc-based index. It's
easy
> to convert between the two (using pc_offset()).
> 
> Or, preferably, replace it with a macro like:
>  PatchableNearJump(byte opcode, Label* label) {
>    db(opcode);
>    emit_near_disp(label);
>  }
> 
> but there is a visibility issue to work around with that, since MacroAssembler
> cant see emit_near_disp. Perhaps make a db_near_disp(Label*) method for that.

I now use set_byte_at and pc_offset instead.

http://codereview.chromium.org/7122003/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1378: inline byte get_opcode(int offset) {
On 2011/06/08 10:58:46, Lasse Reichstein wrote:
> I don't see us using get_opcode anywhere. Just remove it.

It was used in an assert