[Assembler][x64] Make Operand immutable

This CL removes all setters from the Operand and removes the friendship
relation between Assembler and Operand. All data fields of the Operand
are set exactly once in the constructor, the Operand is immutable
afterwards.
In order to construct the data of an Operand easily, the OperandBuilder
is introduced. After building an Operand, the data is copied to the
const field of the Operand.

R=mstarzinger@chromium.org

Bug: v8:7310
Change-Id: I1628052b8a0c47cbfbc3645dfdac5a0e9705977b
Reviewed-on: https://chromium-review.googlesource.com/936741Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51563}

src/compiler/x64/code-generator-x64.cc

@@ -3382,11 +3382,9 @@ void CodeGenerator::AssembleSwap(InstructionOperand* source,
         frame_access_state()->IncreaseSPDelta(1);
         unwinding_info_writer_.MaybeIncreaseBaseOffsetAt(__ pc_offset(),
                                                          kPointerSize);
-        Operand dst = g.ToOperand(destination);
-        __ movq(src, dst);
+        __ movq(src, g.ToOperand(destination));
         frame_access_state()->IncreaseSPDelta(-1);
-        dst = g.ToOperand(destination);
-        __ popq(dst);
+        __ popq(g.ToOperand(destination));
         unwinding_info_writer_.MaybeIncreaseBaseOffsetAt(__ pc_offset(),
                                                          -kPointerSize);
       } else {

src/wasm/baseline/x64/liftoff-assembler-x64.h

@@ -39,6 +39,21 @@ inline Operand GetContextOperand() { return Operand(rbp, -16); }
 // stack for a call to C.
 static constexpr Register kCCallLastArgAddrReg = rax;
 
+inline Operand GetMemOp(LiftoffAssembler* assm, Register addr, Register offset,
+                        uint32_t offset_imm, LiftoffRegList pinned) {
+  if (offset_imm > kMaxInt) {
+    // The immediate can not be encoded in the operand. Load it to a register
+    // first.
+    Register total_offset = assm->GetUnusedRegister(kGpReg, pinned).gp();
+    assm->movl(total_offset, Immediate(offset_imm));
+    if (offset != no_reg) {
+      assm->emit_ptrsize_add(total_offset, addr, offset);
+    }
+    return Operand(addr, total_offset, times_1, 0);
+  }
+  if (offset == no_reg) return Operand(addr, offset_imm);
+  return Operand(addr, offset, times_1, offset_imm);
+}
 }  // namespace liftoff
 
 uint32_t LiftoffAssembler::PrepareStackFrame() {
@@ -110,19 +125,8 @@ void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr,
                             Register offset_reg, uint32_t offset_imm,
                             LoadType type, LiftoffRegList pinned,
                             uint32_t* protected_load_pc) {
-  Operand src_op = offset_reg == no_reg
-                       ? Operand(src_addr, offset_imm)
-                       : Operand(src_addr, offset_reg, times_1, offset_imm);
-  if (offset_imm > kMaxInt) {
-    // The immediate can not be encoded in the operand. Load it to a register
-    // first.
-    Register src = GetUnusedRegister(kGpReg, pinned).gp();
-    movl(src, Immediate(offset_imm));
-    if (offset_reg != no_reg) {
-      emit_ptrsize_add(src, src, offset_reg);
-    }
-    src_op = Operand(src_addr, src, times_1, 0);
-  }
+  Operand src_op =
+      liftoff::GetMemOp(this, src_addr, offset_reg, offset_imm, pinned);
   if (protected_load_pc) *protected_load_pc = pc_offset();
   switch (type.value()) {
     case LoadType::kI32Load8U:
@@ -170,19 +174,8 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,
                              uint32_t offset_imm, LiftoffRegister src,
                              StoreType type, LiftoffRegList pinned,
                              uint32_t* protected_store_pc) {
-  Operand dst_op = offset_reg == no_reg
-                       ? Operand(dst_addr, offset_imm)
-                       : Operand(dst_addr, offset_reg, times_1, offset_imm);
-  if (offset_imm > kMaxInt) {
-    // The immediate can not be encoded in the operand. Load it to a register
-    // first.
-    Register dst = GetUnusedRegister(kGpReg, pinned).gp();
-    movl(dst, Immediate(offset_imm));
-    if (offset_reg != no_reg) {
-      emit_ptrsize_add(dst, dst, offset_reg);
-    }
-    dst_op = Operand(dst_addr, dst, times_1, 0);
-  }
+  Operand dst_op =
+      liftoff::GetMemOp(this, dst_addr, offset_reg, offset_imm, pinned);
   if (protected_store_pc) *protected_store_pc = pc_offset();
   switch (type.value()) {
     case StoreType::kI32Store8:

src/x64/assembler-x64-inl.h

@@ -93,11 +93,11 @@ void Assembler::emit_rex_64(Register reg, XMMRegister rm_reg) {
 }
 
 void Assembler::emit_rex_64(Register reg, Operand op) {
-  emit(0x48 | reg.high_bit() << 2 | op.rex_);
+  emit(0x48 | reg.high_bit() << 2 | op.data().rex);
 }
 
 void Assembler::emit_rex_64(XMMRegister reg, Operand op) {
-  emit(0x48 | (reg.code() & 0x8) >> 1 | op.rex_);
+  emit(0x48 | (reg.code() & 0x8) >> 1 | op.data().rex);
 }
 
 
@@ -106,14 +106,14 @@ void Assembler::emit_rex_64(Register rm_reg) {
   emit(0x48 | rm_reg.high_bit());
 }
 
-void Assembler::emit_rex_64(Operand op) { emit(0x48 | op.rex_); }
+void Assembler::emit_rex_64(Operand op) { emit(0x48 | op.data().rex); }
 
 void Assembler::emit_rex_32(Register reg, Register rm_reg) {
   emit(0x40 | reg.high_bit() << 2 | rm_reg.high_bit());
 }
 
 void Assembler::emit_rex_32(Register reg, Operand op) {
-  emit(0x40 | reg.high_bit() << 2  | op.rex_);
+  emit(0x40 | reg.high_bit() << 2 | op.data().rex);
 }
 
 
@@ -121,7 +121,7 @@ void Assembler::emit_rex_32(Register rm_reg) {
   emit(0x40 | rm_reg.high_bit());
 }
 
-void Assembler::emit_rex_32(Operand op) { emit(0x40 | op.rex_); }
+void Assembler::emit_rex_32(Operand op) { emit(0x40 | op.data().rex); }
 
 void Assembler::emit_optional_rex_32(Register reg, Register rm_reg) {
   byte rex_bits = reg.high_bit() << 2 | rm_reg.high_bit();
@@ -129,12 +129,12 @@ void Assembler::emit_optional_rex_32(Register reg, Register rm_reg) {
 }
 
 void Assembler::emit_optional_rex_32(Register reg, Operand op) {
-  byte rex_bits =  reg.high_bit() << 2 | op.rex_;
+  byte rex_bits = reg.high_bit() << 2 | op.data().rex;
   if (rex_bits != 0) emit(0x40 | rex_bits);
 }
 
 void Assembler::emit_optional_rex_32(XMMRegister reg, Operand op) {
-  byte rex_bits =  (reg.code() & 0x8) >> 1 | op.rex_;
+  byte rex_bits = (reg.code() & 0x8) >> 1 | op.data().rex;
   if (rex_bits != 0) emit(0x40 | rex_bits);
 }
 
@@ -166,7 +166,7 @@ void Assembler::emit_optional_rex_32(XMMRegister rm_reg) {
 }
 
 void Assembler::emit_optional_rex_32(Operand op) {
-  if (op.rex_ != 0) emit(0x40 | op.rex_);
+  if (op.data().rex != 0) emit(0x40 | op.data().rex);
 }
 
 
@@ -180,7 +180,7 @@ void Assembler::emit_vex3_byte1(XMMRegister reg, XMMRegister rm,
 
 // byte 1 of 3-byte VEX
 void Assembler::emit_vex3_byte1(XMMRegister reg, Operand rm, LeadingOpcode m) {
-  byte rxb = ~((reg.high_bit() << 2) | rm.rex_) << 5;
+  byte rxb = ~((reg.high_bit() << 2) | rm.data().rex) << 5;
   emit(rxb | m);
 }
 
@@ -226,7 +226,7 @@ void Assembler::emit_vex_prefix(Register reg, Register vreg, Register rm,
 void Assembler::emit_vex_prefix(XMMRegister reg, XMMRegister vreg, Operand rm,
                                 VectorLength l, SIMDPrefix pp, LeadingOpcode mm,
                                 VexW w) {
-  if (rm.rex_ || mm != k0F || w != kW0) {
+  if (rm.data().rex || mm != k0F || w != kW0) {
     emit_vex3_byte0();
     emit_vex3_byte1(reg, rm, mm);
     emit_vex3_byte2(w, vreg, l, pp);
@@ -413,49 +413,6 @@ void RelocInfo::Visit(ObjectVisitor* visitor) {
   }
 }
 
-// -----------------------------------------------------------------------------
-// Implementation of Operand
-
-void Operand::set_modrm(int mod, Register rm_reg) {
-  DCHECK(is_uint2(mod));
-  buf_[0] = mod << 6 | rm_reg.low_bits();
-  // Set REX.B to the high bit of rm.code().
-  rex_ |= rm_reg.high_bit();
-}
-
-
-void Operand::set_sib(ScaleFactor scale, Register index, Register base) {
-  DCHECK_EQ(len_, 1);
-  DCHECK(is_uint2(scale));
-  // Use SIB with no index register only for base rsp or r12. Otherwise we
-  // would skip the SIB byte entirely.
-  DCHECK(index != rsp || base == rsp || base == r12);
-  buf_[1] = (scale << 6) | (index.low_bits() << 3) | base.low_bits();
-  rex_ |= index.high_bit() << 1 | base.high_bit();
-  len_ = 2;
-}
-
-void Operand::set_disp8(int disp) {
-  DCHECK(is_int8(disp));
-  DCHECK(len_ == 1 || len_ == 2);
-  int8_t* p = reinterpret_cast<int8_t*>(&buf_[len_]);
-  *p = disp;
-  len_ += sizeof(int8_t);
-}
-
-void Operand::set_disp32(int disp) {
-  DCHECK(len_ == 1 || len_ == 2);
-  int32_t* p = reinterpret_cast<int32_t*>(&buf_[len_]);
-  *p = disp;
-  len_ += sizeof(int32_t);
-}
-
-void Operand::set_disp64(int64_t disp) {
-  DCHECK_EQ(1, len_);
-  int64_t* p = reinterpret_cast<int64_t*>(&buf_[len_]);
-  *p = disp;
-  len_ += sizeof(disp);
-}
 }  // namespace internal
 }  // namespace v8
 

src/x64/assembler-x64.h

@@ -333,6 +333,13 @@ enum ScaleFactor : int8_t {
 
 class Operand {
  public:
+  struct Data {
+    byte rex = 0;
+    byte buf[9];
+    byte len = 1;   // number of bytes of buf_ in use.
+    int8_t addend;  // for rip + offset + addend.
+  };
+
   // [base + disp/r]
   Operand(Register base, int32_t disp);
 
@@ -355,41 +362,23 @@ class Operand {
   // [rip + disp/r]
   explicit Operand(Label* label, int addend = 0);
 
+  Operand(const Operand&) = default;
+
   // Checks whether either base or index register is the given register.
   // Does not check the "reg" part of the Operand.
   bool AddressUsesRegister(Register reg) const;
 
   // Queries related to the size of the generated instruction.
   // Whether the generated instruction will have a REX prefix.
-  bool requires_rex() const { return rex_ != 0; }
+  bool requires_rex() const { return data_.rex != 0; }
   // Size of the ModR/M, SIB and displacement parts of the generated
   // instruction.
-  int operand_size() const { return len_; }
-
- private:
-  byte rex_;
-  byte buf_[9];
-  // The number of bytes of buf_ in use.
-  byte len_;
-
-  int8_t addend_;  // for rip + offset + addend
+  int operand_size() const { return data_.len; }
 
-  // Set the ModR/M byte without an encoded 'reg' register. The
-  // register is encoded later as part of the emit_operand operation.
-  // set_modrm can be called before or after set_sib and set_disp*.
-  inline void set_modrm(int mod, Register rm);
+  const Data& data() const { return data_; }
 
-  // Set the SIB byte if one is needed. Sets the length to 2 rather than 1.
-  inline void set_sib(ScaleFactor scale, Register index, Register base);
-
-  // Adds operand displacement fields (offsets added to the memory address).
-  // Needs to be called after set_sib, not before it.
-  inline void set_disp8(int disp);
-  inline void set_disp32(int disp);
-  inline void set_disp64(int64_t disp);  // for labels.
-
-  // TODO(clemensh): Get rid of this friendship, or make Operand immutable.
-  friend class Assembler;
+ private:
+  const Data data_;
 };
 static_assert(sizeof(Operand) <= 2 * kPointerSize,
               "Operand must be small enough to pass it by value");