[Liftoff] [cleanup] Make VarState a proper class

VarState was a struct so far, but gained more and more functionality.
Even more will be added for supporting floating point operations.
Thus, make this a proper class.

Drive-by: Order all switch cases to first handle the stack case, then
register, then constant.

R=titzer@chromium.org

Bug: v8:6600
Change-Id: I694613ebc4910bcf74a1617485bd72878f46e987
Reviewed-on: https://chromium-review.googlesource.com/789937Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49632}

src/wasm/baseline/liftoff-assembler.cc

@@ -16,10 +16,7 @@ namespace v8 {
 namespace internal {
 namespace wasm {
 
-// Note: "State" suffix added to avoid jumbo conflicts with liftoff-compiler.cc
-constexpr auto kRegisterState = LiftoffAssembler::VarState::kRegister;
-constexpr auto kConstantState = LiftoffAssembler::VarState::kConstant;
-constexpr auto kStackState = LiftoffAssembler::VarState::kStack;
+using VarState = LiftoffAssembler::VarState;
 
 namespace {
 
@@ -118,41 +115,41 @@ class StackTransferRecipe {
 
   void TransferStackSlot(const LiftoffAssembler::CacheState& dst_state,
                          uint32_t dst_index, uint32_t src_index) {
-    const LiftoffAssembler::VarState& dst = dst_state.stack_state[dst_index];
-    const LiftoffAssembler::VarState& src =
-        __ cache_state()->stack_state[src_index];
-    switch (dst.loc) {
-      case kConstantState:
-        DCHECK_EQ(dst, src);
-        break;
-      case kRegisterState:
-        switch (src.loc) {
-          case kConstantState:
-            LoadConstant(dst.reg, WasmValue(src.i32_const));
+    const VarState& dst = dst_state.stack_state[dst_index];
+    const VarState& src = __ cache_state()->stack_state[src_index];
+    switch (dst.loc()) {
+      case VarState::kStack:
+        switch (src.loc()) {
+          case VarState::kStack:
+            if (src_index == dst_index) break;
+            // TODO(clemensh): Implement other types than i32.
+            asm_->MoveStackValue(dst_index, src_index, wasm::kWasmI32);
             break;
-          case kRegisterState:
-            if (dst.reg != src.reg) MoveRegister(dst.reg, src.reg);
+          case VarState::kRegister:
+            asm_->Spill(dst_index, src.reg());
             break;
-          case kStackState:
-            LoadStackSlot(dst.reg, src_index);
+          case VarState::kConstant:
+            // TODO(clemensh): Handle other types than i32.
+            asm_->Spill(dst_index, WasmValue(src.i32_const()));
             break;
         }
         break;
-      case kStackState:
-        switch (src.loc) {
-          case kConstantState:
-            // TODO(clemensh): Handle other types than i32.
-            asm_->Spill(dst_index, WasmValue(src.i32_const));
+      case VarState::kRegister:
+        switch (src.loc()) {
+          case VarState::kStack:
+            LoadStackSlot(dst.reg(), src_index);
             break;
-          case kRegisterState:
-            asm_->Spill(dst_index, src.reg);
+          case VarState::kRegister:
+            if (dst.reg() != src.reg()) MoveRegister(dst.reg(), src.reg());
             break;
-          case kStackState:
-            if (src_index == dst_index) break;
-            // TODO(clemensh): Implement other types than i32.
-            asm_->MoveStackValue(dst_index, src_index, wasm::kWasmI32);
+          case VarState::kConstant:
+            LoadConstant(dst.reg(), WasmValue(src.i32_const()));
             break;
         }
+        break;
+      case VarState::kConstant:
+        DCHECK_EQ(dst, src);
+        break;
     }
   }
 
@@ -204,8 +201,8 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
       auto& dst = stack_state[dst_idx];
       auto& src = source.stack_state[src_idx];
       Register reg = no_reg;
-      if (src.is_reg() && is_free(src.reg)) {
-        reg = src.reg;
+      if (src.is_reg() && is_free(src.reg())) {
+        reg = src.reg();
       } else if (has_unused_register()) {
         reg = unused_register();
       } else {
@@ -225,13 +222,13 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
     auto& dst = stack_state[i];
     auto& src = source.stack_state[i];
     if (src.is_reg()) {
-      if (is_used(src.reg)) {
+      if (is_used(src.reg())) {
         // Keep this a stack slot (which is the initial value).
         DCHECK(dst.is_stack());
         continue;
       }
-      dst = VarState(src.reg);
-      inc_used(src.reg);
+      dst = VarState(src.reg());
+      inc_used(src.reg());
     } else if (src.is_const()) {
       dst = src;
     } else {
@@ -272,12 +269,12 @@ LiftoffAssembler::~LiftoffAssembler() {
 Register LiftoffAssembler::GetBinaryOpTargetRegister(
     ValueType type, PinnedRegisterScope pinned_regs) {
   auto& slot_lhs = *(cache_state_.stack_state.end() - 2);
-  if (slot_lhs.loc == kRegisterState && GetNumUses(slot_lhs.reg) == 1) {
-    return slot_lhs.reg;
+  if (slot_lhs.is_reg() && GetNumUses(slot_lhs.reg()) == 1) {
+    return slot_lhs.reg();
   }
   auto& slot_rhs = *(cache_state_.stack_state.end() - 1);
-  if (slot_rhs.loc == kRegisterState && GetNumUses(slot_rhs.reg) == 1) {
-    return slot_rhs.reg;
+  if (slot_rhs.is_reg() && GetNumUses(slot_rhs.reg()) == 1) {
+    return slot_rhs.reg();
   }
   return GetUnusedRegister(type, pinned_regs);
 }
@@ -313,18 +310,18 @@ void LiftoffAssembler::MergeStackWith(CacheState& target, uint32_t arity) {
 
 void LiftoffAssembler::Spill(uint32_t index) {
   auto& slot = cache_state_.stack_state[index];
-  switch (slot.loc) {
-    case kRegisterState:
-      Spill(index, slot.reg);
-      cache_state_.dec_used(slot.reg);
+  switch (slot.loc()) {
+    case VarState::kStack:
+      return;
+    case VarState::kRegister:
+      Spill(index, slot.reg());
+      cache_state_.dec_used(slot.reg());
       break;
-    case kConstantState:
-      Spill(index, WasmValue(slot.i32_const));
+    case VarState::kConstant:
+      Spill(index, WasmValue(slot.i32_const()));
       break;
-    case kStackState:
-      return;
   }
-  slot.loc = kStackState;
+  slot.MakeStack();
 }
 
 void LiftoffAssembler::SpillLocals() {
@@ -338,18 +335,18 @@ Register LiftoffAssembler::PopToRegister(ValueType type,
   DCHECK(!cache_state_.stack_state.empty());
   VarState slot = cache_state_.stack_state.back();
   cache_state_.stack_state.pop_back();
-  switch (slot.loc) {
-    case kRegisterState:
-      cache_state_.dec_used(slot.reg);
-      return slot.reg;
-    case kConstantState: {
+  switch (slot.loc()) {
+    case VarState::kStack: {
       Register reg = GetUnusedRegister(type, pinned_regs);
-      LoadConstant(reg, WasmValue(slot.i32_const));
+      Fill(reg, cache_state_.stack_height());
       return reg;
     }
-    case kStackState: {
+    case VarState::kRegister:
+      cache_state_.dec_used(slot.reg());
+      return slot.reg();
+    case VarState::kConstant: {
       Register reg = GetUnusedRegister(type, pinned_regs);
-      Fill(reg, cache_state_.stack_height());
+      LoadConstant(reg, WasmValue(slot.i32_const()));
       return reg;
     }
   }
@@ -367,9 +364,9 @@ Register LiftoffAssembler::SpillRegister(ValueType type,
   for (uint32_t idx = cache_state_.stack_height() - 1;; --idx) {
     DCHECK_GT(cache_state_.stack_height(), idx);
     auto& slot = cache_state_.stack_state[idx];
-    if (!slot.is_reg() || slot.reg != spill_reg) continue;
+    if (!slot.is_reg() || slot.reg() != spill_reg) continue;
     Spill(idx, spill_reg);
-    slot.loc = kStackState;
+    slot.MakeStack();
     if (--remaining_uses == 0) break;
   }
   cache_state_.register_use_count[spill_reg.code()] = 0;

src/wasm/baseline/liftoff-assembler.h

@@ -76,36 +76,54 @@ class LiftoffAssembler : public TurboAssembler {
 
   Register GetBinaryOpTargetRegister(ValueType, PinnedRegisterScope = {});
 
-  struct VarState {
-    enum Location { kStack, kRegister, kConstant };
-    Location loc;
+  class VarState {
+   public:
+    enum Location : uint8_t { kStack, kRegister, kConstant };
 
-    union {
-      Register reg;
-      uint32_t i32_const;
-    };
-    VarState() : loc(kStack) {}
-    explicit VarState(Register r) : loc(kRegister), reg(r) {}
-    explicit VarState(uint32_t value) : loc(kConstant), i32_const(value) {}
+    VarState() : loc_(kStack) {}
+    explicit VarState(Register r) : loc_(kRegister), reg_(r) {}
+    explicit VarState(uint32_t i32_const)
+        : loc_(kConstant), i32_const_(i32_const) {}
 
     bool operator==(const VarState& other) const {
-      if (loc != other.loc) return false;
-      switch (loc) {
-        case kRegister:
-          return reg == other.reg;
+      if (loc_ != other.loc_) return false;
+      switch (loc_) {
         case kStack:
           return true;
+        case kRegister:
+          return reg_ == other.reg_;
         case kConstant:
-          return i32_const == other.i32_const;
+          return i32_const_ == other.i32_const_;
       }
       UNREACHABLE();
     }
 
-    bool is_stack() const { return loc == kStack; }
-    bool is_reg() const { return loc == kRegister; }
-    bool is_const() const { return loc == kConstant; }
-  };
+    bool is_stack() const { return loc_ == kStack; }
+    bool is_reg() const { return loc_ == kRegister; }
+    bool is_const() const { return loc_ == kConstant; }
+
+    Location loc() const { return loc_; }
+
+    uint32_t i32_const() const {
+      DCHECK_EQ(loc_, kConstant);
+      return i32_const_;
+    }
 
+    Register reg() const {
+      DCHECK_EQ(loc_, kRegister);
+      return reg_;
+    }
+
+    void MakeStack() { loc_ = kStack; }
+
+   private:
+    Location loc_;
+
+    union {
+      Register reg_;        // used if loc_ == kRegister
+      uint32_t i32_const_;  // used if loc_ == kConstant
+    };
+  };
   static_assert(IS_TRIVIALLY_COPYABLE(VarState),
                 "VarState should be trivially copyable");
 
@@ -216,11 +234,11 @@ class LiftoffAssembler : public TurboAssembler {
   void DropStackSlot(VarState* slot) {
     // The only loc we care about is register. Other types don't occupy
     // anything.
-    if (slot->loc != VarState::kRegister) return;
+    if (!slot->is_reg()) return;
     // Free the register, then set the loc to "stack".
     // No need to write back, the value should be dropped.
-    cache_state_.dec_used(slot->reg);
-    slot->loc = VarState::kStack;
+    cache_state_.dec_used(slot->reg());
+    slot->MakeStack();
   }
 
   void MergeFullStackWith(CacheState&);

src/wasm/baseline/liftoff-compiler.cc

@@ -308,12 +308,12 @@ class LiftoffCompiler {
   void GetLocal(Decoder* decoder, Value* result,
                 const LocalIndexOperand<validate>& operand) {
     auto& slot = __ cache_state()->stack_state[operand.index];
-    switch (slot.loc) {
+    switch (slot.loc()) {
       case kRegister:
-        __ PushRegister(slot.reg);
+        __ PushRegister(slot.reg());
         break;
       case kConstant:
-        __ cache_state()->stack_state.emplace_back(slot.i32_const);
+        __ cache_state()->stack_state.emplace_back(slot.i32_const());
         break;
       case kStack: {
         Register reg = __ GetUnusedRegister(__ local_type(operand.index));
@@ -328,24 +328,24 @@ class LiftoffCompiler {
     auto& state = *__ cache_state();
     auto& source_slot = state.stack_state.back();
     auto& target_slot = state.stack_state[local_index];
-    switch (source_slot.loc) {
+    switch (source_slot.loc()) {
       case kRegister:
         __ DropStackSlot(&target_slot);
         target_slot = source_slot;
-        if (is_tee) state.inc_used(target_slot.reg);
+        if (is_tee) state.inc_used(target_slot.reg());
         break;
       case kConstant:
         __ DropStackSlot(&target_slot);
         target_slot = source_slot;
         break;
       case kStack: {
-        switch (target_slot.loc) {
+        switch (target_slot.loc()) {
           case kRegister:
-            if (state.register_use_count[target_slot.reg.code()] == 1) {
-              __ Fill(target_slot.reg, state.stack_height() - 1);
+            if (state.register_use_count[target_slot.reg().code()] == 1) {
+              __ Fill(target_slot.reg(), state.stack_height() - 1);
               break;
             } else {
-              state.dec_used(target_slot.reg);
+              state.dec_used(target_slot.reg());
               // and fall through to use a new register.
             }
           case kConstant: