Fix bug in X64 RSet code. Optimize IA32 version.

src/ia32/macro-assembler-ia32.cc

 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 38 matching lines @@
 
 
 static void RecordWriteHelper(MacroAssembler* masm,
                               Register object,
                               Register addr,
                               Register scratch) {
   Label fast;
 
   // Compute the page address from the heap object pointer, leave it
   // in 'object'.
   masm->and_(object, ~Page::kPageAlignmentMask);

[Kevin Millikin (Chromium), 2009/08/05 10:26:15]

This whole function is probably clearer if we introduce a symbolic alias for
'object'.  Something like 'base', 'start', 'page_start'?

 
   // Compute the bit addr in the remembered set, leave it in "addr".
   masm->sub(addr, Operand(object));
   masm->shr(addr, kObjectAlignmentBits);
 
   // If the bit offset lies beyond the normal remembered set range, it is in
   // the extra remembered set area of a large object.
   masm->cmp(addr, Page::kPageSize / kPointerSize);
   masm->j(less, &fast);
 
   // Adjust 'addr' to be relative to the start of the extra remembered set

[William Hesse, 2009/08/05 10:17:50]

This comment is no longer right.  I would comment
"Store the length of the fixed array in object."
and 
"Let object be the starting address of the extra remembered set, minus
kRSetOffset and minus an offset of the remembered set bits which are stored in
the first part of the remembered set, but still counted in addr.
The two offsets add up to kRSetEndOffset.

[Kevin Millikin (Chromium), 2009/08/05 10:26:15]

I would ditch the whole comment and use symbolic aliases for the register names
to make it more clear.

   // and the page address in 'object' to be the address of the extra
   // remembered set.
-  masm->sub(Operand(addr), Immediate(Page::kPageSize / kPointerSize));
-  // Load the array length into 'scratch' and multiply by four to get the
-  // size in bytes of the elements.
+
+  // Find the length of the FixedArray.
   masm->mov(scratch, Operand(object, Page::kObjectStartOffset
                                      + FixedArray::kLengthOffset));
-  masm->shl(scratch, kObjectAlignmentBits);
-  // Add the page header, array header, and array body size to the page
-  // address.
-  masm->add(Operand(object), Immediate(Page::kObjectStartOffset
-                                       + FixedArray::kHeaderSize));
-  masm->add(object, Operand(scratch));
 
-
+  // The RSet extension area lies after the FixedArray, i.e.,

[William Hesse, 2009/08/05 10:17:50]

how about "after the FixedArray, at"

+  // at
+  //   object + kObjectStartOffset + FixedArray::kHeaderSize + 8 * scratch

[Kevin Millikin (Chromium), 2009/08/05 10:26:15]

Surely not 8?

+  // Make object point to (size of normal RSet) before that, so that we
+  // can address the bit directly with addr.
+  masm->lea(object,
+            Operand(object, scratch, times_pointer_size,
+                    Page::kObjectStartOffset + FixedArray::kHeaderSize
+                        - Page::kRSetEndOffset));
   // NOTE: For now, we use the bit-test-and-set (bts) x86 instruction
   // to limit code size. We should probably evaluate this decision by
   // measuring the performance of an equivalent implementation using
   // "simpler" instructions
   masm->bind(&fast);
-  masm->bts(Operand(object, 0), addr);
+  masm->bts(Operand(object, Page::kRSetOffset), addr);
 }
 
 
 class RecordWriteStub : public CodeStub {
  public:
   RecordWriteStub(Register object, Register addr, Register scratch)
       : object_(object), addr_(addr), scratch_(scratch) { }
 
   void Generate(MacroAssembler* masm);
 
@@ skipping 927 matching lines @@
   // Indicate that code has changed.
   CPU::FlushICache(address_, size_);
 
   // Check that the code was patched as expected.
   ASSERT(masm_.pc_ == address_ + size_);
   ASSERT(masm_.reloc_info_writer.pos() == address_ + size_ + Assembler::kGap);
 }
 
 
 } }  // namespace v8::internal