Fix bug in X64 RSet code. Optimize IA32 version.

Review URL: http://codereview.chromium.org/162001


git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@2626 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/ia32/macro-assembler-ia32.cc

@@ -54,41 +54,47 @@ static void RecordWriteHelper(MacroAssembler* masm,
                               Register scratch) {
   Label fast;
 
-  // Compute the page address from the heap object pointer, leave it
-  // in 'object'.
+  // Compute the page start address from the heap object pointer, and reuse
+  // the 'object' register for it.
   masm->and_(object, ~Page::kPageAlignmentMask);
+  Register page_start = object;
 
-  // Compute the bit addr in the remembered set, leave it in "addr".
-  masm->sub(addr, Operand(object));
+  // Compute the bit addr in the remembered set/index of the pointer in the
+  // page. Reuse 'addr' as pointer_offset.
+  masm->sub(addr, Operand(page_start));
   masm->shr(addr, kObjectAlignmentBits);
+  Register pointer_offset = addr;
 
   // 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->cmp(pointer_offset, Page::kPageSize / kPointerSize);
   masm->j(less, &fast);
 
-  // Adjust 'addr' to be relative to the start of the extra remembered set
-  // 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.
-  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));
-
+  // Adjust 'page_start' so that addressing using 'pointer_offset' hits the
+  // extra remembered set after the large object.
+
+  // Find the length of the large object (FixedArray).
+  masm->mov(scratch, Operand(page_start, Page::kObjectStartOffset
+                                         + FixedArray::kLengthOffset));
+  Register array_length = scratch;
+
+  // Extra remembered set starts right after the large object (a FixedArray), at
+  //   page_start + kObjectStartOffset + objectSize
+  // where objectSize is FixedArray::kHeaderSize + kPointerSize * array_length.
+  // Add the delta between the end of the normal RSet and the start of the
+  // extra RSet to 'object', so that addressing the bit using 'pointer_offset'
+  // hits the extra RSet words.
+  masm->lea(page_start,
+            Operand(page_start, array_length, 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(page_start, Page::kRSetOffset), pointer_offset);
 }
 
 

src/spaces-inl.h

@@ -127,20 +127,19 @@ Address Page::ComputeRSetBitPosition(Address address, int offset,
 
   if (rset_address >= page->RSetEnd()) {
     // We have a large object page, and the remembered set address is actually
-    // past the end of the object.  The address of the remembered set in this
-    // case is the extra remembered set start address at the address of the
-    // end of the object:
+    // past the end of the object.
+
+    // The first part of the remembered set is still located at the start of
+    // the page, but anything after kRSetEndOffset must be relocated to after
+    // the large object, i.e. after
     //   (page->ObjectAreaStart() + object size)
-    // plus the offset of the computed remembered set address from the start
-    // of the object:
-    //   (rset_address - page->ObjectAreaStart()).
-    // Ie, we can just add the object size.
-    // In the X64 architecture, the remembered set ends before the object start,
-    // so we need to add an additional offset, from rset end to object start
+    // We do that by adding the difference between the normal RSet's end and
+    // the object's end.
     ASSERT(HeapObject::FromAddress(address)->IsFixedArray());
-    rset_address += kObjectStartOffset - kRSetEndOffset +
+    int fixedarray_length =
         FixedArray::SizeFor(Memory::int_at(page->ObjectAreaStart()
                                            + Array::kLengthOffset));
+    rset_address += kObjectStartOffset - kRSetEndOffset + fixedarray_length;
   }
   return rset_address;
 }

src/x64/macro-assembler-x64.cc

@@ -53,43 +53,50 @@ static void RecordWriteHelper(MacroAssembler* masm,
                               Register scratch) {
   Label fast;
 
-  // Compute the page address from the heap object pointer, leave it
-  // in 'object'.
+  // Compute the page start address from the heap object pointer, and reuse
+  // the 'object' register for it.
   ASSERT(is_int32(~Page::kPageAlignmentMask));
   masm->and_(object,
              Immediate(static_cast<int32_t>(~Page::kPageAlignmentMask)));
+  Register page_start = object;
 
-  // Compute the bit addr in the remembered set, leave it in "addr".
-  masm->subq(addr, object);
+  // Compute the bit addr in the remembered set/index of the pointer in the
+  // page. Reuse 'addr' as pointer_offset.
+  masm->subq(addr, page_start);
   masm->shr(addr, Immediate(kPointerSizeLog2));
+  Register pointer_offset = addr;
 
   // If the bit offset lies beyond the normal remembered set range, it is in
   // the extra remembered set area of a large object.
-  masm->cmpq(addr, Immediate(Page::kPageSize / kPointerSize));
+  masm->cmpq(pointer_offset, Immediate(Page::kPageSize / kPointerSize));
   masm->j(less, &fast);
 
-  // Adjust 'addr' to be relative to the start of the extra remembered set
-  // and the page address in 'object' to be the address of the extra
-  // remembered set.
-  masm->subq(addr, Immediate(Page::kPageSize / kPointerSize));
+  // Adjust 'page_start' so that addressing using 'pointer_offset' hits the
+  // extra remembered set after the large object.
+
   // Load the array length into 'scratch'.
   masm->movl(scratch,
-             Operand(object,
+             Operand(page_start,
                      Page::kObjectStartOffset + FixedArray::kLengthOffset));
-  // Extra remembered set starts right after FixedArray.
-  // Add the page header, array header, and array body size
-  // (length * pointer size) to the page address to find the extra remembered
-  // set start.
-  masm->lea(object,
-            Operand(object, scratch, times_pointer_size,
-                    Page::kObjectStartOffset + FixedArray::kHeaderSize));
+  Register array_length = scratch;
+
+  // Extra remembered set starts right after the large object (a FixedArray), at
+  //   page_start + kObjectStartOffset + objectSize
+  // where objectSize is FixedArray::kHeaderSize + kPointerSize * array_length.
+  // Add the delta between the end of the normal RSet and the start of the
+  // extra RSet to 'page_start', so that addressing the bit using
+  // 'pointer_offset' hits the extra RSet words.
+  masm->lea(page_start,
+            Operand(page_start, array_length, 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, Page::kRSetOffset), addr);
+  masm->bts(Operand(page_start, Page::kRSetOffset), pointer_offset);
 }
 
 
@@ -181,7 +188,7 @@ void MacroAssembler::RecordWrite(Register object,
     } else {
       // array access: calculate the destination address in the same manner as
       // KeyedStoreIC::GenerateGeneric.  Multiply a smi by 4 to get an offset
-      // into an array of words.
+      // into an array of pointers.
       lea(dst, Operand(object, dst, times_half_pointer_size,
                        FixedArray::kHeaderSize - kHeapObjectTag));
     }

test/mozilla/mozilla.status

@@ -816,15 +816,3 @@ js1_5/extensions/regress-336410-2: CRASH
 js1_5/extensions/regress-336410-1: CRASH
 js1_5/Function/regress-338001: FAIL || CRASH
 js1_5/extensions/regress-371636: CRASH
-# The following failures were added when remembered sets were enabled.
-js1_5/GC/regress-203278-2: FAIL || PASS || CRASH
-js1_5/GC/regress-203278-3: FAIL || PASS
-js1_5/Regress/regress-280769-3: FAIL || PASS
-js1_5/Regress/regress-280769-4: CRASH || TIMEOUT
-js1_5/Regress/regress-290575: CRASH
-js1_5/extensions/regress-365692: FAIL || PASS
-js1_5/Regress/regress-366601: FAIL
-js1_5/Regress/regress-367561-03: CRASH
-js1_5/Regress/regress-367561-01: CRASH || PASS
-ecma/Expressions/11.7.2: CRASH
-