Clean up ARM assembly code loading and storing from/to a large offset.

runtime/vm/assembler_arm.cc

Index: runtime/vm/assembler_arm.cc
===================================================================
--- runtime/vm/assembler_arm.cc	(revision 19530)
+++ runtime/vm/assembler_arm.cc	(working copy)
@@ -12,8 +12,7 @@
 
 namespace dart {
 
-// TODO(regis): Enable this flag after PrintStopMessage stub is implemented.
-DEFINE_FLAG(bool, print_stop_message, false, "Print stop message.");
+DEFINE_FLAG(bool, print_stop_message, true, "Print stop message.");
 
 
 bool CPUFeatures::integer_division_supported_ = false;
@@ -1249,13 +1248,14 @@
   }
   const int32_t offset =
       Array::data_offset() + 4*AddObject(object) - kHeapObjectTag;
-  if (Address::CanHoldLoadOffset(kLoadWord, offset)) {
+  int32_t offset_mask;
+  if (Address::CanHoldLoadOffset(kLoadWord, offset, &offset_mask)) {
     ldr(rd, Address(PP, offset));
   } else {
-    int32_t offset12_hi = offset & ~kOffset12Mask;  // signed
-    uint32_t offset12_lo = offset & kOffset12Mask;  // unsigned
-    AddImmediate(rd, PP, offset12_hi);
-    ldr(rd, Address(rd, offset12_lo));
+    int32_t offset_hi = offset & ~offset_mask;  // signed
+    uint32_t offset_lo = offset & offset_mask;  // unsigned

[zra, 2013/03/06 16:41:27]

ASSERT(CanHoldLoadOffset(offset_lo))

[regis, 2013/03/06 18:42:00]

Actually, each instruction already asserts that the offset is in range. So
instead of adding extra asserts here, I prefer to remove those in LoadFromOffset
and friends.

+    AddImmediate(rd, PP, offset_hi);
+    ldr(rd, Address(rd, offset_lo));
   }
 }
 
@@ -1274,40 +1274,58 @@
 }
 
 
-bool Address::CanHoldLoadOffset(LoadOperandType type, int offset) {
+bool Address::CanHoldLoadOffset(LoadOperandType type,

[zra, 2013/03/06 03:30:28]

I just want to double-check this and CanHoldStoreOffset against the spec first
thing tomorrow morning, but otherwise I think these changes are good.

[regis, 2013/03/06 18:42:00]

OK. I'll revisit if you find a problem.

+                                int32_t offset,
+                                int32_t* offset_mask) {
   switch (type) {
     case kLoadSignedByte:
     case kLoadSignedHalfword:
     case kLoadUnsignedHalfword:
-    case kLoadWordPair:
+    case kLoadWordPair: {
+      *offset_mask = 0xff;
       return Utils::IsAbsoluteUint(8, offset);  // Addressing mode 3.
+    }
     case kLoadUnsignedByte:
-    case kLoadWord:
+    case kLoadWord: {
+      *offset_mask = 0xfff;
       return Utils::IsAbsoluteUint(12, offset);  // Addressing mode 2.
+    }
     case kLoadSWord:
-    case kLoadDWord:
+    case kLoadDWord: {
+      *offset_mask = 0x3ff;
       return Utils::IsAbsoluteUint(10, offset);  // VFP addressing mode.
-    default:
+    }
+    default: {
       UNREACHABLE();
       return false;
+    }
   }
 }
 
 
-bool Address::CanHoldStoreOffset(StoreOperandType type, int offset) {
+bool Address::CanHoldStoreOffset(StoreOperandType type,
+                                 int32_t offset,
+                                 int32_t* offset_mask) {
   switch (type) {
     case kStoreHalfword:
-    case kStoreWordPair:
+    case kStoreWordPair: {
+      *offset_mask = 0xff;
       return Utils::IsAbsoluteUint(8, offset);  // Addressing mode 3.
+    }
     case kStoreByte:
-    case kStoreWord:
+    case kStoreWord: {
+      *offset_mask = 0xfff;
       return Utils::IsAbsoluteUint(12, offset);  // Addressing mode 2.
+    }
     case kStoreSWord:
-    case kStoreDWord:
+    case kStoreDWord: {
+      *offset_mask = 0x3ff;
       return Utils::IsAbsoluteUint(10, offset);  // VFP addressing mode.
-    default:
+    }
+    default: {
       UNREACHABLE();
       return false;
+    }
   }
 }
 
@@ -1393,24 +1411,25 @@
   // use 'blx ip' in a non-patchable sequence (see other BranchLink flavors).
   const int32_t offset =
       Array::data_offset() + 4*AddExternalLabel(label) - kHeapObjectTag;
-  if (Address::CanHoldLoadOffset(kLoadWord, offset)) {
+  int32_t offset_mask;
+  if (Address::CanHoldLoadOffset(kLoadWord, offset, &offset_mask)) {
     ldr(LR, Address(PP, offset));
   } else {
-    int32_t offset12_hi = offset & ~kOffset12Mask;  // signed
-    uint32_t offset12_lo = offset & kOffset12Mask;  // unsigned
-    // Inline a simplified version of AddImmediate(LR, CP, offset12_hi).
+    int32_t offset_hi = offset & ~offset_mask;  // signed
+    uint32_t offset_lo = offset & offset_mask;  // unsigned
+    // Inline a simplified version of AddImmediate(LR, CP, offset_hi).
     ShifterOperand shifter_op;
-    if (ShifterOperand::CanHold(offset12_hi, &shifter_op)) {
+    if (ShifterOperand::CanHold(offset_hi, &shifter_op)) {
       add(LR, PP, shifter_op);
     } else {
-      movw(LR, Utils::Low16Bits(offset12_hi));
-      const uint16_t value_high = Utils::High16Bits(offset12_hi);
+      movw(LR, Utils::Low16Bits(offset_hi));
+      const uint16_t value_high = Utils::High16Bits(offset_hi);
       if (value_high != 0) {
         movt(LR, value_high);
       }
       add(LR, PP, ShifterOperand(LR));
     }
-    ldr(LR, Address(LR, offset12_lo));
+    ldr(LR, Address(LR, offset_lo));
   }
   blx(LR);  // Use blx instruction so that the return branch prediction works.
 }
@@ -1427,11 +1446,12 @@
 void Assembler::BranchLinkOffset(Register base, int offset) {
   ASSERT(base != PC);
   ASSERT(base != IP);
-  if (Address::CanHoldLoadOffset(kLoadWord, offset)) {
+  int32_t offset_mask;
+  if (Address::CanHoldLoadOffset(kLoadWord, offset, &offset_mask)) {
     ldr(IP, Address(base, offset));
   } else {
-    int offset_hi = offset & ~kOffset12Mask;
-    int offset_lo = offset & kOffset12Mask;
+    int offset_hi = offset & ~offset_mask;
+    int offset_lo = offset & offset_mask;

[zra, 2013/03/06 16:41:27]

For consistency, you might put ASSERT(CanHoldLoadOffset(..., offset_lo)) here
also like you have down in LoadFromOffset. Same in BranchLinkPatchable.

[regis, 2013/03/06 18:42:00]

I am all for consistency. So I'll remove the extra ones, instead of adding
redundant ones.

     ShifterOperand offset_hi_op;
     if (ShifterOperand::CanHold(offset_hi, &offset_hi_op)) {
       add(IP, base, offset_hi_op);
@@ -1493,14 +1513,14 @@
                                Register base,
                                int32_t offset,
                                Condition cond) {
-  if (!Address::CanHoldLoadOffset(type, offset)) {
+  int32_t offset_mask;
+  if (!Address::CanHoldLoadOffset(type, offset, &offset_mask)) {
     ASSERT(base != IP);
-    LoadImmediate(IP, offset, cond);
-    add(IP, IP, ShifterOperand(base), cond);
+    AddImmediate(IP, base, offset & ~offset_mask, cond);
     base = IP;
-    offset = 0;
+    offset = offset & offset_mask;
   }
-  ASSERT(Address::CanHoldLoadOffset(type, offset));
+  ASSERT(Address::CanHoldLoadOffset(type, offset, &offset_mask));
   switch (type) {
     case kLoadSignedByte:
       ldrsb(reg, Address(base, offset), cond);
@@ -1531,15 +1551,15 @@
                               Register base,
                               int32_t offset,
                               Condition cond) {
-  if (!Address::CanHoldStoreOffset(type, offset)) {
+  int32_t offset_mask;
+  if (!Address::CanHoldStoreOffset(type, offset, &offset_mask)) {
     ASSERT(reg != IP);
     ASSERT(base != IP);
-    LoadImmediate(IP, offset, cond);
-    add(IP, IP, ShifterOperand(base), cond);
+    AddImmediate(IP, base, offset & ~offset_mask, cond);
     base = IP;
-    offset = 0;
+    offset = offset & offset_mask;
   }
-  ASSERT(Address::CanHoldStoreOffset(type, offset));
+  ASSERT(Address::CanHoldStoreOffset(type, offset, &offset_mask));
   switch (type) {
     case kStoreByte:
       strb(reg, Address(base, offset), cond);
@@ -1563,14 +1583,14 @@
                                 Register base,
                                 int32_t offset,
                                 Condition cond) {
-  if (!Address::CanHoldLoadOffset(kLoadSWord, offset)) {
+  int32_t offset_mask;
+  if (!Address::CanHoldLoadOffset(kLoadSWord, offset, &offset_mask)) {
     ASSERT(base != IP);
-    LoadImmediate(IP, offset, cond);
-    add(IP, IP, ShifterOperand(base), cond);
+    AddImmediate(IP, base, offset & ~offset_mask, cond);
     base = IP;
-    offset = 0;
+    offset = offset & offset_mask;
   }
-  ASSERT(Address::CanHoldLoadOffset(kLoadSWord, offset));
+  ASSERT(Address::CanHoldLoadOffset(kLoadSWord, offset, &offset_mask));
   vldrs(reg, Address(base, offset), cond);
 }
 
@@ -1579,14 +1599,14 @@
                                Register base,
                                int32_t offset,
                                Condition cond) {
-  if (!Address::CanHoldStoreOffset(kStoreSWord, offset)) {
+  int32_t offset_mask;
+  if (!Address::CanHoldStoreOffset(kStoreSWord, offset, &offset_mask)) {
     ASSERT(base != IP);
-    LoadImmediate(IP, offset, cond);
-    add(IP, IP, ShifterOperand(base), cond);
+    AddImmediate(IP, base, offset & ~offset_mask, cond);
     base = IP;
-    offset = 0;
+    offset = offset & offset_mask;
   }
-  ASSERT(Address::CanHoldStoreOffset(kStoreSWord, offset));
+  ASSERT(Address::CanHoldStoreOffset(kStoreSWord, offset, &offset_mask));
   vstrs(reg, Address(base, offset), cond);
 }
 
@@ -1595,14 +1615,14 @@
                                 Register base,
                                 int32_t offset,
                                 Condition cond) {
-  if (!Address::CanHoldLoadOffset(kLoadDWord, offset)) {
+  int32_t offset_mask;
+  if (!Address::CanHoldLoadOffset(kLoadDWord, offset, &offset_mask)) {
     ASSERT(base != IP);
-    LoadImmediate(IP, offset, cond);
-    add(IP, IP, ShifterOperand(base), cond);
+    AddImmediate(IP, base, offset & ~offset_mask, cond);
     base = IP;
-    offset = 0;
+    offset = offset & offset_mask;
   }
-  ASSERT(Address::CanHoldLoadOffset(kLoadDWord, offset));
+  ASSERT(Address::CanHoldLoadOffset(kLoadDWord, offset, &offset_mask));
   vldrd(reg, Address(base, offset), cond);
 }
 
@@ -1611,14 +1631,14 @@
                                Register base,
                                int32_t offset,
                                Condition cond) {
-  if (!Address::CanHoldStoreOffset(kStoreDWord, offset)) {
+  int32_t offset_mask;
+  if (!Address::CanHoldStoreOffset(kStoreDWord, offset, &offset_mask)) {
     ASSERT(base != IP);
-    LoadImmediate(IP, offset, cond);
-    add(IP, IP, ShifterOperand(base), cond);
+    AddImmediate(IP, base, offset & ~offset_mask, cond);
     base = IP;
-    offset = 0;
+    offset = offset & offset_mask;
   }
-  ASSERT(Address::CanHoldStoreOffset(kStoreDWord, offset));
+  ASSERT(Address::CanHoldStoreOffset(kStoreDWord, offset, &offset_mask));
   vstrd(reg, Address(base, offset), cond);
 }
 
@@ -1784,8 +1804,7 @@
   EnterFrame(kDartVolatileCpuRegs | (1 << FP), 0);
 
   // Preserve all volatile FPU registers.
-  // TODO(regis): Use vstmd instruction once supported.
-  // vstmd(DB_W, SP, kDartFirstVolatileFpuReg, kDartLastVolatileFpuReg);
+  vstmd(DB_W, SP, kDartFirstVolatileFpuReg, kDartLastVolatileFpuReg);
 
   ReserveAlignedFrameSpace(frame_space);
 }
@@ -1796,13 +1815,12 @@
   // and ensure proper alignment of the stack frame.
   // We need to restore it before restoring registers.
   const intptr_t kPushedRegistersSize =
-      kDartVolatileCpuRegCount * kWordSize;
-  // TODO(regis): + kDartVolatileFpuRegCount * 2 * kWordSize;
+      kDartVolatileCpuRegCount * kWordSize +
+      kDartVolatileFpuRegCount * 2 * kWordSize;
   AddImmediate(SP, FP, -kPushedRegistersSize);
 
   // Restore all volatile FPU registers.
-  // TODO(regis): Use vldmd instruction once supported.
-  // vldmd(IA_W, SP, kDartFirstVolatileFpuReg, kDartLastVolatileFpuReg);
+  vldmd(IA_W, SP, kDartFirstVolatileFpuReg, kDartLastVolatileFpuReg);
 
   // Restore volatile CPU registers.
   LeaveFrame(kDartVolatileCpuRegs | (1 << FP));