Revert "[Assembler][x64] Make Operand immutable"

This reverts commit e7f9fb4a.

Reason for revert: msvc compile error: https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20msvc/builds/1573

Original change's description:
> [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/936741
> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#51563}

TBR=mstarzinger@chromium.org,clemensh@chromium.org

Change-Id: I8ae40de35e81765549f93ffe58f1b12286de6333
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:7310
Reviewed-on: https://chromium-review.googlesource.com/936627Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51564}

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

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

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

@@ -39,21 +39,6 @@ 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() {
@@ -125,8 +110,19 @@ 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 =
-      liftoff::GetMemOp(this, src_addr, offset_reg, offset_imm, pinned);
+  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);
+  }
   if (protected_load_pc) *protected_load_pc = pc_offset();
   switch (type.value()) {
     case LoadType::kI32Load8U:
@@ -174,8 +170,19 @@ 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 =
-      liftoff::GetMemOp(this, dst_addr, offset_reg, offset_imm, pinned);
+  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);
+  }
   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.data().rex);
+  emit(0x48 | reg.high_bit() << 2 | op.rex_);
 }
 
 void Assembler::emit_rex_64(XMMRegister reg, Operand op) {
-  emit(0x48 | (reg.code() & 0x8) >> 1 | op.data().rex);
+  emit(0x48 | (reg.code() & 0x8) >> 1 | op.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.data().rex); }
+void Assembler::emit_rex_64(Operand op) { emit(0x48 | op.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.data().rex);
+  emit(0x40 | reg.high_bit() << 2  | op.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.data().rex); }
+void Assembler::emit_rex_32(Operand op) { emit(0x40 | op.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.data().rex;
+  byte rex_bits =  reg.high_bit() << 2 | op.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.data().rex;
+  byte rex_bits =  (reg.code() & 0x8) >> 1 | op.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.data().rex != 0) emit(0x40 | op.data().rex);
+  if (op.rex_ != 0) emit(0x40 | op.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.data().rex) << 5;
+  byte rxb = ~((reg.high_bit() << 2) | rm.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.data().rex || mm != k0F || w != kW0) {
+  if (rm.rex_ || mm != k0F || w != kW0) {
     emit_vex3_byte0();
     emit_vex3_byte1(reg, rm, mm);
     emit_vex3_byte2(w, vreg, l, pp);
@@ -413,6 +413,49 @@ 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,13 +333,6 @@ 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);
 
@@ -362,23 +355,41 @@ 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 data_.rex != 0; }
+  bool requires_rex() const { return rex_ != 0; }
   // Size of the ModR/M, SIB and displacement parts of the generated
   // instruction.
-  int operand_size() const { return data_.len; }
-
-  const Data& data() const { return data_; }
+  int operand_size() const { return len_; }
 
  private:
-  const Data data_;
+  byte rex_;
+  byte buf_[9];
+  // The number of bytes of buf_ in use.
+  byte len_;
+
+  int8_t addend_;  // for rip + offset + addend
+
+  // 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);
+
+  // 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;
 };
 static_assert(sizeof(Operand) <= 2 * kPointerSize,
               "Operand must be small enough to pass it by value");