[arm] Introduce UseScratchRegisterScope

Introduce a stripped down version of UseScratchRegisterScope for ARM and use it
inside the assembler and macro-assembler. At the exception of the Call
instructions, we now use this scope instead of using the ip register
directly. This is inspired from how the ARM64 backend works.

In general, the benefit of doing this is we can catch cases where ip is being
used both by the caller and by the assembler. But more specifically, TurboFan
reserves r9 as an extra scratch register because ip can already be used by the
assembler. With this utility, we can isolate the cases in the code generator
which need an extra register and potentially fix them, allowing us to give r9
back to the register allocator.

This patch uncovered places in the assembler where we were using ip
unconditionally when we could have re-used the destination register instead.

Bug: v8:6553
Change-Id: Ib7134e3ed64dd1f90baf209ae831ed8f644cac78
Reviewed-on: https://chromium-review.googlesource.com/544956
Commit-Queue: Pierre Langlois <pierre.langlois@arm.com>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46425}

src/arm/assembler-arm.h

@@ -546,8 +546,8 @@ class Operand BASE_EMBEDDED {
 
   // Return the number of actual instructions required to implement the given
   // instruction for this particular operand. This can be a single instruction,
-  // if no load into the ip register is necessary, or anything between 2 and 4
-  // instructions when we need to load from the constant pool (depending upon
+  // if no load into a scratch register is necessary, or anything between 2 and
+  // 4 instructions when we need to load from the constant pool (depending upon
   // whether the constant pool entry is in the small or extended section). If
   // the instruction this operand is used for is a MOV or MVN instruction the
   // actual instruction to use is required for this calculation. For other
@@ -607,8 +607,9 @@ class MemOperand BASE_EMBEDDED {
   // [rn +/- offset]      Offset/NegOffset
   // [rn +/- offset]!     PreIndex/NegPreIndex
   // [rn], +/- offset     PostIndex/NegPostIndex
-  // offset is any signed 32-bit value; offset is first loaded to register ip if
-  // it does not fit the addressing mode (12-bit unsigned and sign bit)
+  // offset is any signed 32-bit value; offset is first loaded to a scratch
+  // register if it does not fit the addressing mode (12-bit unsigned and sign
+  // bit)
   explicit MemOperand(Register rn, int32_t offset = 0, AddrMode am = Offset);
 
   // [rn +/- rm]          Offset/NegOffset
@@ -823,6 +824,8 @@ class Assembler : public AssemblerBase {
   static constexpr int kDebugBreakSlotLength =
       kDebugBreakSlotInstructions * kInstrSize;
 
+  RegList* GetScratchRegisterList() { return &scratch_register_list_; }
+
   // ---------------------------------------------------------------------------
   // Code generation
 
@@ -1168,7 +1171,7 @@ class Assembler : public AssemblerBase {
   void vmov(const SwVfpRegister dst, float imm);
   void vmov(const DwVfpRegister dst,
             Double imm,
-            const Register scratch = no_reg);
+            const Register extra_scratch = no_reg);
   void vmov(const SwVfpRegister dst,
             const SwVfpRegister src,
             const Condition cond = al);
@@ -1788,6 +1791,9 @@ class Assembler : public AssemblerBase {
   // Map of address of handle to index in pending_32_bit_constants_.
   std::map<Address, int> handle_to_index_map_;
 
+  // Scratch registers available for use by the Assembler.
+  RegList scratch_register_list_;
+
  private:
   // Avoid overflows for displacements etc.
   static const int kMaximalBufferSize = 512 * MB;
@@ -1896,6 +1902,29 @@ class PatchingAssembler : public Assembler {
   void FlushICache(Isolate* isolate);
 };
 
+// This scope utility allows scratch registers to be managed safely. The
+// Assembler's GetScratchRegisterList() is used as a pool of scratch
+// registers. These registers can be allocated on demand, and will be returned
+// at the end of the scope.
+//
+// When the scope ends, the Assembler's list will be restored to its original
+// state, even if the list is modified by some other means. Note that this scope
+// can be nested but the destructors need to run in the opposite order as the
+// constructors. We do not have assertions for this.
+class UseScratchRegisterScope {
+ public:
+  explicit UseScratchRegisterScope(Assembler* assembler);
+  ~UseScratchRegisterScope();
+
+  // Take a register from the list and return it.
+  Register Acquire();
+
+ private:
+  // Currently available scratch registers.
+  RegList* available_;
+  // Available scratch registers at the start of this scope.
+  RegList old_available_;
+};
 
 }  // namespace internal
 }  // namespace v8

src/arm/macro-assembler-arm.h

@@ -817,7 +817,7 @@ class MacroAssembler: public Assembler {
   // are the same register).  It leaves the heap object in the heap_object
   // register unless the heap_object register is the same register as one of the
   // other registers.
-  // Type_reg can be no_reg. In that case ip is used.
+  // Type_reg can be no_reg. In that case a scratch register is used.
   void CompareObjectType(Register heap_object,
                          Register map,
                          Register type_reg,
@@ -869,11 +869,13 @@ class MacroAssembler: public Assembler {
   void LoadWeakValue(Register value, Handle<WeakCell> cell, Label* miss);
 
   // Compare the object in a register to a value from the root list.
-  // Uses the ip register as scratch.
+  // Acquires a scratch register.
   void CompareRoot(Register obj, Heap::RootListIndex index);
   void PushRoot(Heap::RootListIndex index) {
-    LoadRoot(ip, index);
-    Push(ip);
+    UseScratchRegisterScope temps(this);
+    Register scratch = temps.Acquire();
+    LoadRoot(scratch, index);
+    Push(scratch);
   }
 
   // Compare the object in a register to a value and jump if they are equal.
@@ -1090,9 +1092,9 @@ class MacroAssembler: public Assembler {
     return code_object_;
   }
 
-
   // Emit code for a truncating division by a constant. The dividend register is
-  // unchanged and ip gets clobbered. Dividend and result must be different.
+  // unchanged and a scratch register needs to be available. Dividend and result
+  // must be different.
   void TruncatingDiv(Register result, Register dividend, int32_t divisor);
 
   // ---------------------------------------------------------------------------
@@ -1168,9 +1170,11 @@ class MacroAssembler: public Assembler {
     TrySmiTag(reg, reg, not_a_smi);
   }
   void TrySmiTag(Register reg, Register src, Label* not_a_smi) {
-    SmiTag(ip, src, SetCC);
+    UseScratchRegisterScope temps(this);
+    Register scratch = temps.Acquire();
+    SmiTag(scratch, src, SetCC);
     b(vs, not_a_smi);
-    mov(reg, ip);
+    mov(reg, scratch);
   }
 
 

src/arm64/macro-assembler-arm64.h

@@ -2602,14 +2602,15 @@ class InstructionAccurateScope BASE_EMBEDDED {
 #endif
 };
 
-
 // This scope utility allows scratch registers to be managed safely. The
 // MacroAssembler's TmpList() (and FPTmpList()) is used as a pool of scratch
 // registers. These registers can be allocated on demand, and will be returned
 // at the end of the scope.
 //
 // When the scope ends, the MacroAssembler's lists will be restored to their
-// original state, even if the lists were modified by some other means.
+// original state, even if the lists were modified by some other means. Note
+// that this scope can be nested but the destructors need to run in the opposite
+// order as the constructors. We do not have assertions for this.
 class UseScratchRegisterScope {
  public:
   explicit UseScratchRegisterScope(MacroAssembler* masm)

test/cctest/test-assembler-arm.cc

@@ -3969,4 +3969,26 @@ TEST(regress4292_CheckConstPool) {
   __ vldr(d0, MemOperand(r0, 0));
 }
 
+TEST(use_scratch_register_scope) {
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+  HandleScope scope(isolate);
+
+  Assembler assm(isolate, NULL, 0);
+
+  // The assembler should have ip as a scratch by default.
+  CHECK_EQ(*assm.GetScratchRegisterList(), ip.bit());
+
+  {
+    UseScratchRegisterScope temps(&assm);
+    CHECK_EQ(*assm.GetScratchRegisterList(), ip.bit());
+
+    Register scratch = temps.Acquire();
+    CHECK_EQ(scratch.code(), ip.code());
+    CHECK_EQ(*assm.GetScratchRegisterList(), 0);
+  }
+
+  CHECK_EQ(*assm.GetScratchRegisterList(), ip.bit());
+}
+
 #undef __

test/cctest/test-disasm-arm.cc

@@ -40,26 +40,38 @@
 
 using namespace v8::internal;
 
-
-bool DisassembleAndCompare(byte* pc, const char* compare_string) {
+template <typename... S>
+bool DisassembleAndCompare(byte* begin, S... expected_strings) {
   disasm::NameConverter converter;
   disasm::Disassembler disasm(converter);
-  EmbeddedVector<char, 128> disasm_buffer;
-
-  disasm.InstructionDecode(disasm_buffer, pc);
-
-  if (strcmp(compare_string, disasm_buffer.start()) != 0) {
-    fprintf(stderr,
-            "expected: \n"
-            "%s\n"
-            "disassembled: \n"
-            "%s\n\n",
-            compare_string, disasm_buffer.start());
-    return false;
+  EmbeddedVector<char, 128> buffer;
+
+  std::vector<std::string> expected_disassembly = {expected_strings...};
+  size_t n_expected = expected_disassembly.size();
+  byte* end = begin + (n_expected * Assembler::kInstrSize);
+
+  std::vector<std::string> disassembly;
+  for (byte* pc = begin; pc < end;) {
+    pc += disasm.InstructionDecode(buffer, pc);
+    disassembly.emplace_back(buffer.start());
   }
-  return true;
-}
 
+  bool test_passed = true;
+
+  for (size_t i = 0; i < disassembly.size(); i++) {
+    if (expected_disassembly[i] != disassembly[i]) {
+      fprintf(stderr,
+              "expected: \n"
+              "%s\n"
+              "disassembled: \n"
+              "%s\n\n",
+              expected_disassembly[i].c_str(), disassembly[i].c_str());
+      test_passed = false;
+    }
+  }
+
+  return test_passed;
+}
 
 // Set up V8 to a state where we can at least run the assembler and
 // disassembler. Declare the variables and allocate the data structures used
@@ -77,12 +89,12 @@ bool DisassembleAndCompare(byte* pc, const char* compare_string) {
 // disassembles the generated instruction, comparing the output to the expected
 // value. If the comparison fails an error message is printed, but the test
 // continues to run until the end.
-#define COMPARE(asm_, compare_string) \
-  { \
-    int pc_offset = assm.pc_offset(); \
-    byte *progcounter = &buffer[pc_offset]; \
-    assm.asm_; \
-    if (!DisassembleAndCompare(progcounter, compare_string)) failure = true; \
+#define COMPARE(asm_, ...)                                                \
+  {                                                                       \
+    int pc_offset = assm.pc_offset();                                     \
+    byte* progcounter = &buffer[pc_offset];                               \
+    assm.asm_;                                                            \
+    if (!DisassembleAndCompare(progcounter, __VA_ARGS__)) failure = true; \
   }
 
 // Force emission of any pending literals into a pool.
@@ -272,21 +284,22 @@ TEST(Type0) {
   if (CpuFeatures::IsSupported(ARMv7)) {
     COMPARE(mov(r5, Operand(0x01234), LeaveCC, ne),
             "13015234       movwne r5, #4660");
-    // We only disassemble one instruction so the eor instruction is not here.
     COMPARE(eor(r5, r4, Operand(0x1234), LeaveCC, ne),
-            "1301c234       movwne ip, #4660");
-    // Movw can't do setcc, so first move to ip, then the following instruction
-    // moves to r5.  Mov immediate with setcc is pretty strange anyway.
+            "13015234       movwne r5, #4660",
+            "10245005       eorne r5, r4, r5");
+    // Movw can't do setcc, so first move to r5, then the following instruction
+    // sets the flags. Mov immediate with setcc is pretty strange anyway.
     COMPARE(mov(r5, Operand(0x01234), SetCC, ne),
-            "1301c234       movwne ip, #4660");
+            "13015234       movwne r5, #4660",
+            "11b05005       movnes r5, r5");
     // Emit a literal pool now, otherwise this could be dumped later, in the
     // middle of a different test.
     EMIT_PENDING_LITERALS();
 
-    // We only disassemble one instruction so the eor instruction is not here.
     // The eor does the setcc so we get a movw here.
     COMPARE(eor(r5, r4, Operand(0x1234), SetCC, ne),
-            "1301c234       movwne ip, #4660");
+            "13015234       movwne r5, #4660",
+            "10345005       eornes r5, r4, r5");
 
     COMPARE(movt(r5, 0x4321, ne),
             "13445321       movtne r5, #17185");
@@ -297,7 +310,8 @@ TEST(Type0) {
   // Eor doesn't have an eor-negative variant, but we can do an mvn followed by
   // an eor to get the same effect.
   COMPARE(eor(r5, r4, Operand(0xffffff34), SetCC, ne),
-          "13e0c0cb       mvnne ip, #203");
+          "13e050cb       mvnne r5, #203",
+          "10345005       eornes r5, r4, r5");
 
   // and <-> bic.
   COMPARE(and_(r3, r5, Operand(0xfc03ffff)),
@@ -1402,6 +1416,42 @@ TEST(LoadStore) {
             "f5d2f080       pld [r2, #+128]");
   }
 
+  // Test out-of-bound immediates.
+  COMPARE(ldrb(r6, MemOperand(r7, 42 << 12)),
+          "e3a06a2a       mov r6, #172032",
+          "e7d76006       ldrb r6, [r7, +r6]");
+  COMPARE(ldrh(r6, MemOperand(r7, 42 << 8, PostIndex)),
+          "e3a06c2a       mov r6, #10752",
+          "e09760b6       ldrh r6, [r7], +r6");
+  // Make sure ip is used if the destination is the same as the base.
+  COMPARE(ldr(r8, MemOperand(r8, 42 << 12, PreIndex)),
+          "e3a0ca2a       mov ip, #172032",
+          "e7b8800c       ldr r8, [r8, +ip]!");
+  COMPARE(strb(r6, MemOperand(r7, 42 << 12)),
+          "e3a0ca2a       mov ip, #172032",
+          "e7c7600c       strb r6, [r7, +ip]");
+  COMPARE(strh(r6, MemOperand(r7, 42 << 8, PostIndex)),
+          "e3a0cc2a       mov ip, #10752",
+          "e08760bc       strh r6, [r7], +ip");
+  COMPARE(str(r6, MemOperand(r7, 42 << 12, PreIndex)),
+          "e3a0ca2a       mov ip, #172032",
+          "e7a7600c       str r6, [r7, +ip]!");
+
+  // Test scaled operands for instructions that do not support it natively.
+  COMPARE(ldrh(r0, MemOperand(r1, r2, LSL, 2)),
+          "e1a00102       mov r0, r2, lsl #2",
+          "e19100b0       ldrh r0, [r1, +r0]");
+  COMPARE(strh(r3, MemOperand(r4, r5, LSR, 3)),
+          "e1a0c1a5       mov ip, r5, lsr #3",
+          "e18430bc       strh r3, [r4, +ip]");
+  // Make sure ip is used if the destination is the same as the base.
+  COMPARE(ldrsb(r6, MemOperand(r6, r8, ASR, 4)),
+          "e1a0c248       mov ip, r8, asr #4",
+          "e19660dc       ldrsb r6, [r6, +ip]");
+  COMPARE(ldrsh(r9, MemOperand(sp, r10, ROR, 5)),
+          "e1a092ea       mov r9, r10, ror #5",
+          "e19d90f9       ldrsh r9, [sp, +r9]");
+
   VERIFY_RUN();
 }