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#newcode1378
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#newcode5189
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#newcode5190
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#newcode197
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#newcode3151
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#newcode3152
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#newcode2237
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#newcode344
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#newcode3741
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#newcode3793
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.
http://codereview.chromium.org/7122003/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
