Replace the constant pool access ldr instructions with movw/movt, and remove ...

src/arm/assembler-arm-inl.h

Index: src/arm/assembler-arm-inl.h
===================================================================
--- src/arm/assembler-arm-inl.h	(revision 4311)
+++ src/arm/assembler-arm-inl.h	(working copy)
@@ -73,9 +73,9 @@
 }
 
 
-void RelocInfo::set_target_address(Address target) {
+void RelocInfo::set_target_address(Address target, bool need_icache_flush) {
   ASSERT(IsCodeTarget(rmode_) || rmode_ == RUNTIME_ENTRY);
-  Assembler::set_target_address_at(pc_, target);
+  Assembler::set_target_address_at(pc_, target, need_icache_flush);
 }
 
 
@@ -84,10 +84,31 @@
   return Memory::Object_at(Assembler::target_address_address_at(pc_));
 }
 
+static uint32_t get_movw_movt_address(Address pc) {
+  Instr instr = Memory::int32_at(pc);
+  ASSERT((instr & kMovwMovtMask) == kMovwPattern);
+  Instr instr_movt = Memory::int32_at(pc + Assembler::kInstrSize);
+  int i = 0;
+  while ((instr_movt & kMovwMovtMask) != kMovtPattern) {
+    i++;
+    instr_movt = Memory::int32_at(pc + Assembler::kInstrSize * i);
+  }
+  ASSERT((instr_movt & kMovwMovtMask) == kMovtPattern);
+  uint32_t offset = ((instr & 0xf0000) >> 4) | (instr & 0xfff);

[Erik Corry, 2010/05/03 10:06:28]

The name 'offset' seems wrong here.  It's the immediate from the movw-movt
instruction.  The word 'offset' makes it sound like it is relative to something.

+  int32_t offset_movt = ((instr_movt & 0xf0000) >> 4) | (instr_movt & 0xfff);
+  offset = offset | offset_movt << 16;
+  return offset;
+}
 
+
 Handle<Object> RelocInfo::target_object_handle(Assembler* origin) {
   ASSERT(IsCodeTarget(rmode_) || rmode_ == EMBEDDED_OBJECT);
-  return Memory::Object_Handle_at(Assembler::target_address_address_at(pc_));
+  Instr instr = Memory::int32_at(pc_);
+  if ((instr & kLdrMask) == kLdrPattern) {
+    return Memory::Object_Handle_at(Assembler::target_address_address_at(pc_));
+  } else {
+    return reinterpret_cast<Object**>(get_movw_movt_address(pc_));

[Erik Corry, 2010/05/03 10:06:28]

This seems wrong.  This function is expected to return a Handle, which is
implemented internally as a pointer to a pointer to an object.  This line takes
a pointer to and object and casts it to add the extra indirection.  Incidentally
this line makes use of the implicit Handle constructor that takes an Object**. 
I have made that constructor explicit since it violated our style guide.

I think the correct change is to have this function return an Object* instead of
a Handle<Object>.  It seems like the only place we use this (objects.cc) we
immediately get the Object* out of the Handle anyway.

+  }
 }
 
 
@@ -97,9 +118,10 @@
 }
 
 
-void RelocInfo::set_target_object(Object* target) {
+void RelocInfo::set_target_object(Object* target, bool need_icache_flush) {
   ASSERT(IsCodeTarget(rmode_) || rmode_ == EMBEDDED_OBJECT);
-  Assembler::set_target_address_at(pc_, reinterpret_cast<Address>(target));
+  Assembler::set_target_address_at(pc_, reinterpret_cast<Address>(target),

[Erik Corry, 2010/05/03 10:06:28]

If all arguments don't fit on one line it is neater to give them one line each
so that it visually looks like 3 arguments and not two.

+                                   need_icache_flush);
 }
 
 
@@ -244,19 +266,26 @@
   }
 #endif
 
-  // Verify that the instruction to patch is a
-  // ldr<cond> <Rd>, [pc +/- offset_12].
-  ASSERT((instr & 0x0f7f0000) == 0x051f0000);
-  int offset = instr & 0xfff;  // offset_12 is unsigned
-  if ((instr & (1 << 23)) == 0) offset = -offset;  // U bit defines offset sign
-  // Verify that the constant pool comes after the instruction referencing it.
-  ASSERT(offset >= -4);
-  return target_pc + offset + 8;
+  if ((instr & kLdrMask) == kLdrPattern) {
+    int offset = instr & 0xfff;  // offset_12 is unsigned
+    // U bit defines offset sign
+    if ((instr & (1 << 23)) == 0) offset = -offset;
+    // Verify that the constant pool comes after the instruction referencing it.
+    ASSERT(offset >= -4);
+    return pc + offset + 8;
+  } else {
+    return (Address) get_movw_movt_address(pc);
+  }
 }
 
 
 Address Assembler::target_address_at(Address pc) {
-  return Memory::Address_at(target_address_address_at(pc));
+  Instr instr = Memory::int32_at(pc);
+  if ((instr & kLdrMask) == kLdrPattern) {
+    return Memory::Address_at(target_address_address_at(pc));
+  } else {
+    return (Address) get_movw_movt_address(pc);
+  }
 }
 
 
@@ -266,8 +295,31 @@
 }
 
 
-void Assembler::set_target_address_at(Address pc, Address target) {
-  Memory::Address_at(target_address_address_at(pc)) = target;
+void Assembler::set_target_address_at(Address pc, Address target,

[Erik Corry, 2010/05/03 10:06:28]

Please put arguments either on one line or on a line each.

+                                      bool need_icache_flush) {
+  Instr instr = Memory::int32_at(pc);
+  if ((instr & kLdrMask) == kLdrPattern) {
+    Memory::Address_at(target_address_address_at(pc)) = target;
+  } else {
+    // movw movt instructions.
+    uint32_t* new_pc = reinterpret_cast<uint32_t*>(pc);
+    uint32_t new_addr = reinterpret_cast<uint32_t>(target);
+    *new_pc = (*new_pc & 0xfff0f000) | ((new_addr & 0xf000) << 4) |

[Erik Corry, 2010/05/03 10:06:28]

Please give this constant, 0xfff0ff000, a name and use it throughout.

Also, there is a function that returns ((x & 0xf000) << 4) | (x & 0xfff). 
Please use it throughout.

+        (new_addr & 0xfff);
+    Instr instr_movt = Memory::int32_at(pc + Assembler::kInstrSize);
+    int i = 1;
+    while ((instr_movt & kMovwMovtMask) != kMovtPattern) {
+      i++;
+      instr_movt = Memory::int32_at(pc + Assembler::kInstrSize * i);
+    }
+    ASSERT((instr_movt & kMovwMovtMask) == kMovtPattern);
+    int32_t new_movt_addr = new_addr >> 16;
+    *(new_pc + i) = (*(new_pc + i) & 0xfff0f000) |
+        ((new_movt_addr & 0xf000) << 4) | (new_movt_addr & 0xfff);
+    if (need_icache_flush)

[Erik Corry, 2010/05/03 10:06:28]

Please use braces on multiline if statements.  This isn't a Google style guide
requirement, but we try to stick to it on V8.

+      CPU::FlushICache(pc, sizeof(target));

[Erik Corry, 2010/05/03 10:06:28]

sizeof(target) looks wrong here.  With the loop above, the size to be flushed is
not a constant.

+  }
+
   // Intuitively, we would think it is necessary to flush the instruction cache
   // after patching a target address in the code as follows:
   //   CPU::FlushICache(pc, sizeof(target));