[Decomp] Add SSA based check for expression building (#292)

* wip

* tests pass

* fix warnings

decompiler/analysis/atomic_op_builder.cpp

@@ -44,15 +44,15 @@ Register rv0() {
 // Variable Helpers
 /////////////////////////
 
-Variable make_dst_var(Register reg, int idx) {
-  return Variable(VariableMode::WRITE, reg, idx);
+RegisterAccess make_dst_var(Register reg, int idx) {
+  return RegisterAccess(AccessMode::WRITE, reg, idx);
 }
 
-Variable make_src_var(Register reg, int idx) {
-  return Variable(VariableMode::READ, reg, idx);
+RegisterAccess make_src_var(Register reg, int idx) {
+  return RegisterAccess(AccessMode::READ, reg, idx);
 }
 
-Variable make_dst_var(const Instruction& i, int idx) {
+RegisterAccess make_dst_var(const Instruction& i, int idx) {
   assert(i.n_dst == 1);
   return make_dst_var(i.get_dst(0).get_reg(), idx);
 }

decompiler/analysis/cfg_builder.cpp

@@ -277,7 +277,7 @@ bool delay_slot_sets_truthy(BranchElement* branch, SetVarOp& delay) {
  */
 bool try_clean_up_sc_as_and(FormPool& pool, Function& func, ShortCircuitElement* ir) {
   Register destination;
-  Variable ir_dest;
+  RegisterAccess ir_dest;
   for (int i = 0; i < int(ir->entries.size()) - 1; i++) {
     auto branch = get_condition_branch(ir->entries.at(i).condition);
     assert(branch.first);
@@ -318,6 +318,35 @@ bool try_clean_up_sc_as_and(FormPool& pool, Function& func, ShortCircuitElement*
         branch_info.consumes.insert(x);
       }
 
+      auto delay_op = func.ir2.atomic_ops->ops.at(delay_id).get();
+      auto as_set = dynamic_cast<SetVarOp*>(delay_op);
+      assert(as_set);
+      if (as_set->src().is_var()) {
+        // must be the case where the src should have truthy in it.
+        // lg::warn("Disabling use of {} in or delay slot", as_set->to_string(func.ir2.env));
+        func.ir2.env.disable_use(as_set->src().var());
+      }
+
+      // we also want to fix up the use/def info for the result.
+      // it's somewhat arbitrary, but we use the convention that the short-circuit defs
+      // are eliminated:
+      auto& ud_info = func.ir2.env.get_use_def_info(as_set->dst());
+      if (i == int(ir->entries.size()) - 2) {
+        if (ud_info.def_count() == 1) {
+          // the final case of the or doesn't explicitly set the destination register.
+          // this can happen if the move is eliminated during coloring.
+          // for now, let's leave this last def here, just so it looks like _something_ sets it.
+
+        } else {
+          // lg::warn("Disabling def of {} in final or delay slot",
+          // as_set->to_string(func.ir2.env));
+          func.ir2.env.disable_def(as_set->dst());
+        }
+      } else {
+        // lg::warn("Disabling def of {} in or delay slot", as_set->to_string(func.ir2.env));
+        func.ir2.env.disable_def(as_set->dst());
+      }
+
       if (i == 0) {
         live_out_result = (branch_info.written_and_unused.find(ir_dest.reg()) ==
                            branch_info.written_and_unused.end());
@@ -346,12 +375,20 @@ bool try_clean_up_sc_as_and(FormPool& pool, Function& func, ShortCircuitElement*
  * Note - this will convert an and to a very strange or, so always use the try as and first.
  */
 bool try_clean_up_sc_as_or(FormPool& pool, Function& func, ShortCircuitElement* ir) {
-  Register destination;
-  Variable ir_dest;
+  // all cases of an or, excluding the last one, should move the value "true" into the result reg
+  // in case the short circuit is taken.  The final case should just write the result reg.
+
+  Register destination;    // destination register
+  RegisterAccess ir_dest;  // destination access (the first one)
+
+  // all but the last one (these should all do the delay slot trick)
+  // this first pass is where we can reject this as an or.
   for (int i = 0; i < int(ir->entries.size()) - 1; i++) {
+    // short circuit branch
     auto branch = get_condition_branch(ir->entries.at(i).condition);
     assert(branch.first);
     assert(ir->entries.at(i).branch_delay.has_value());
+    // the branch should write true (there's two ways this can happen)
     if (!delay_slot_sets_truthy(branch.first, *ir->entries.at(i).branch_delay)) {
       return false;
     }
@@ -367,9 +404,19 @@ bool try_clean_up_sc_as_or(FormPool& pool, Function& func, ShortCircuitElement*
     }
   }
 
+  // at this point, we know that all non-last cases write the destination, and what
+  // the destination is.
+  // so we commit to rewriting this thing as an or, and any errors from here on are fatal.
+
   ir->kind = ShortCircuitElement::OR;
   ir->final_result = ir_dest;
 
+  // we also would like to know if the result of this OR is used or not.
+  // there's also a sanity check that:
+  //  if the result is used - all writes to the result reg _should_ be live
+  //  if the result is unused - all writes to the result reg should be dead.
+  //  otherwise it means that our control flow graph is messed up, and we abort.
+
   bool live_out_result = false;
 
   for (int i = 0; i < int(ir->entries.size()) - 1; i++) {
@@ -380,12 +427,48 @@ bool try_clean_up_sc_as_or(FormPool& pool, Function& func, ShortCircuitElement*
       auto delay_id = ir->entries.at(i).branch_delay->dst().idx();
       auto& delay_info = func.ir2.env.reg_use().op.at(delay_id);
 
+      // cheat for the old method
       auto branch_id = branch.first->op()->op_id();
       auto& branch_info = func.ir2.env.reg_use().op.at(branch_id);
       for (auto x : delay_info.consumes) {
         branch_info.consumes.insert(x);
       }
 
+      // the branch may look like this:
+      // bnel s7, a3, L283
+      // or a2, a3, r0
+      // which reads a3 twice.  But we want it to count as only once, as the second read
+      // is inserted by the GOAL compiler, not by putting a var twice in the source code.
+      auto delay_op = func.ir2.atomic_ops->ops.at(delay_id).get();
+      auto as_set = dynamic_cast<SetVarOp*>(delay_op);
+      assert(as_set);
+      if (as_set->src().is_var()) {
+        // must be the case where the src should have truthy in it.
+        // lg::warn("Disabling use of {} in or delay slot", as_set->to_string(func.ir2.env));
+        func.ir2.env.disable_use(as_set->src().var());
+      }
+
+      // we also want to fix up the use/def info for the result.
+      // it's somewhat arbitrary, but we use the convention that the short-circuit defs
+      // are eliminated:
+      auto& ud_info = func.ir2.env.get_use_def_info(as_set->dst());
+      if (i == int(ir->entries.size()) - 2) {
+        if (ud_info.def_count() == 1) {
+          // the final case of the or doesn't explicitly set the destination register.
+          // this can happen if the move is eliminated during coloring.
+          // for now, let's leave this last def here, just so it looks like _something_ sets it.
+          // TODO - what if this isn't a def in the last slot? Does it matter?
+        } else {
+          // lg::warn("Disabling def of {} in final or delay slot",
+          // as_set->to_string(func.ir2.env));
+          func.ir2.env.disable_def(as_set->dst());
+        }
+
+      } else {
+        // lg::warn("Disabling def of {} in or delay slot", as_set->to_string(func.ir2.env));
+        func.ir2.env.disable_def(as_set->dst());
+      }
+
       if (i == 0) {
         live_out_result = (delay_info.written_and_unused.find(ir_dest.reg()) ==
                            delay_info.written_and_unused.end());
@@ -400,7 +483,10 @@ bool try_clean_up_sc_as_or(FormPool& pool, Function& func, ShortCircuitElement*
     *(branch.second) = replacement;
   }
 
+  // TODO - check the one remaining def location?
+
   ir->used_as_value = live_out_result;
+
   return true;
 }
 
@@ -505,7 +591,7 @@ const SimpleAtom* get_atom_src(const Form* form) {
  * successfully
  */
 void convert_cond_no_else_to_compare(FormPool& pool,
-                                     const Function& f,
+                                     Function& f,
                                      FormElement** ir_loc,
                                      Form* parent_form) {
   CondNoElseElement* cne = dynamic_cast<CondNoElseElement*>(*ir_loc);
@@ -521,6 +607,9 @@ void convert_cond_no_else_to_compare(FormPool& pool,
   assert(src_atom->get_str() == "#f");
   assert(cne->entries.size() == 1);
 
+  // safe to do this here because we never give up on this.
+  f.ir2.env.disable_def(condition.first->op()->branch_delay().var(0));
+
   auto condition_as_single =
       dynamic_cast<BranchElement*>(cne->entries.front().condition->try_as_single_element());
   auto condition_replacement = condition.first->op()->get_condition_as_form(pool, f.ir2.env);
@@ -556,7 +645,7 @@ void convert_cond_no_else_to_compare(FormPool& pool,
   }
 }
 
-void clean_up_cond_no_else_final(const Function& func, CondNoElseElement* cne) {
+void clean_up_cond_no_else_final(Function& func, CondNoElseElement* cne) {
   for (size_t idx = 0; idx < cne->entries.size(); idx++) {
     auto& entry = cne->entries.at(idx);
     if (entry.false_destination.has_value()) {
@@ -589,6 +678,15 @@ void clean_up_cond_no_else_final(const Function& func, CondNoElseElement* cne) {
              branch_info_i.written_and_unused.end());
     }
   }
+
+  for (size_t i = 0; i < cne->entries.size(); i++) {
+    if (func.ir2.env.has_reg_use()) {
+      auto branch = dynamic_cast<BranchElement*>(cne->entries.at(i).original_condition_branch);
+      auto reg = cne->entries.at(i).false_destination;
+      // lg::warn("Disable def of {} at {}\n", reg->to_string(func.ir2.env), reg->idx());
+      func.ir2.env.disable_def(*reg);
+    }
+  }
 }
 
 /*!
@@ -600,10 +698,7 @@ void clean_up_cond_no_else_final(const Function& func, CondNoElseElement* cne) {
  * But it generally seems inconsistent.  The expression propagation step will have to deal with
  * this.
  */
-void clean_up_cond_no_else(FormPool& pool,
-                           const Function& f,
-                           FormElement** ir_loc,
-                           Form* parent_form) {
+void clean_up_cond_no_else(FormPool& pool, Function& f, FormElement** ir_loc, Form* parent_form) {
   auto cne = dynamic_cast<CondNoElseElement*>(*ir_loc);
   assert(cne);
   for (size_t idx = 0; idx < cne->entries.size(); idx++) {
@@ -980,7 +1075,7 @@ Form* try_sc_as_ash(FormPool& pool, Function& f, const ShortCircuit* vtx) {
     return nullptr;
   }
 
-  std::optional<Variable> clobber_ir;
+  std::optional<RegisterAccess> clobber_ir;
   auto dsubu_set = dynamic_cast<SetVarElement*>(dsubu_candidate);
   auto dsrav_set = dynamic_cast<SetVarElement*>(dsrav_candidate);
   assert(dsubu_set && dsrav_set);
@@ -988,7 +1083,7 @@ Form* try_sc_as_ash(FormPool& pool, Function& f, const ShortCircuit* vtx) {
     clobber_ir = dsubu_set->dst();
   }
 
-  Variable dest_ir = result;
+  RegisterAccess dest_ir = result;
   SimpleAtom shift_ir = branch->op()->condition().src(0);
   auto value_ir =
       dynamic_cast<const SimpleExpressionElement*>(dsrav_set->src()->try_as_single_element())
@@ -1016,6 +1111,9 @@ Form* try_sc_as_ash(FormPool& pool, Function& f, const ShortCircuit* vtx) {
   auto set_form = pool.alloc_element<SetVarElement>(dest_ir, ash_form, true);
   b0_c_ptr->push_back(set_form);
 
+  // fix up reg info
+  f.ir2.env.disable_use(delay->src().get_arg(0).var());
+
   return b0_c_ptr;
 }
 
@@ -1147,7 +1245,7 @@ Form* try_sc_as_type_of(FormPool& pool, Function& f, const ShortCircuit* vtx) {
   assert(src_reg3.var().reg() == src_reg.reg());
   assert(offset.get_int() == -4);
 
-  std::optional<Variable> clobber;
+  std::optional<RegisterAccess> clobber;
   if (temp_reg.reg() != dst_reg.reg()) {
     clobber = first_branch->op()->condition().src(0).var();
   }
@@ -1157,12 +1255,17 @@ Form* try_sc_as_type_of(FormPool& pool, Function& f, const ShortCircuit* vtx) {
   // remove the shift
   b0_ptr->pop_back();
 
+  // add the type-of
   auto obj = pool.alloc_single_element_form<SimpleExpressionElement>(
       nullptr, shift->expr().get_arg(0).as_expr(), set_shift->dst().idx());
   auto type_op = pool.alloc_single_element_form<TypeOfElement>(nullptr, obj, clobber);
   auto op = pool.alloc_element<SetVarElement>(else_case->dst(), type_op, true);
   b0_ptr->push_back(op);
-  // add the type-of
+
+  // fix register info
+  f.ir2.env.disable_def(b0_delay_op.dst());
+  f.ir2.env.disable_def(b1_delay_op.dst());
+  f.ir2.env.disable_use(shift->expr().get_arg(0).var());
 
   return b0_ptr;
 }

decompiler/analysis/variable_naming.cpp

@@ -22,7 +22,7 @@ std::string reg_to_string(const T& regs) {
 
 /*!
  * Allocate a new SSA variable for the given register.
- * This should only be used to allocate the result of a non-phi instruction.
+ * This should only be used to allocate the result of a non-phi instruction (a real instruction)
  */
 VarSSA VarMapSSA::allocate(Register reg) {
   Entry new_entry;
@@ -35,7 +35,7 @@ VarSSA VarMapSSA::allocate(Register reg) {
 }
 
 /*!
- * Allocate a new SSA for the given register.
+ * Allocate a new SSA variable for the given register as a result of a phi.
  * This should only be used to allocate the result of a phi-function.
  */
 VarSSA VarMapSSA::allocate_init_phi(Register reg, int block_id) {
@@ -128,13 +128,16 @@ bool VarMapSSA::same(const VarSSA& var_a, const VarSSA& var_b) const {
 /*!
  * Get program variable ID from an SSA variable.
  */
-int VarMapSSA::var_id(const VarSSA& var) {
+int VarMapSSA::var_id(const VarSSA& var) const {
   return m_entries.at(var.m_entry_id).var_id;
 }
 
 /*!
  * For a given register and map, remap using var_id = remap[var_id]
  * For variables not in the map, set ID to INT32_MIN.
+ *
+ * This allows you to do a full remapping, without worrying new/old mappings aliasing part way
+ * through the remapping.
  */
 void VarMapSSA::remap_reg(Register reg, const std::unordered_map<int, int>& remap) {
   for (auto& entry : m_entries) {
@@ -337,11 +340,13 @@ SSA make_rc_ssa(const Function& function, const RegUsageInfo& rui, const Functio
         got_not_arg_coloring = true;
         auto as_set = dynamic_cast<const SetVarOp*>(op.get());
         if (as_set) {
+          auto dst = as_set->dst().reg();
+
           if ((as_set->src().kind() == SimpleExpression::Kind::GPR_TO_FPR ||
                as_set->src().is_identity()) &&
               as_set->src().get_arg(0).is_var()) {
             auto src = as_set->src().get_arg(0).var().reg();
-            auto dst = as_set->dst().reg();
+
             if (is_possible_coloring_move(dst, src) &&
                 rui.op.at(op_id).consumes.find(src) != rui.op.at(op_id).consumes.end()) {
               ssa_i.is_arg_coloring_move = true;
@@ -351,6 +356,19 @@ SSA make_rc_ssa(const Function& function, const RegUsageInfo& rui, const Functio
         }
       }
 
+      auto as_set = dynamic_cast<const SetVarOp*>(op.get());
+      if (as_set) {
+        auto dst = as_set->dst().reg();
+        if (as_set->src().is_var()) {
+          auto src = as_set->src().get_arg(0).var().reg();
+          auto& ri = rui.op.at(op_id);
+          if (ri.consumes.find(src) != ri.consumes.end() &&
+              ri.written_and_unused.find(dst) != ri.written_and_unused.end()) {
+            ssa_i.is_dead_set = true;
+          }
+        }
+      }
+
       // todo - verify no duplicates here?
       assert(op->write_regs().size() <= 1);
       // reads:
@@ -480,6 +498,10 @@ void SSA::merge_all_phis() {
   }
 }
 
+/*!
+ * Remaps all SSA variable ids to final variable IDs.
+ * This forces you to have all positive, consecutive IDs, with 0 being the entry value.
+ */
 void SSA::remap() {
   // this keeps the order of variable assignments in the instruction order, not var_id order.
   struct VarIdRecord {
@@ -528,6 +550,15 @@ void SSA::remap() {
     for (auto var_id : reg_vars.second.order) {
       var_remap[var_id] = i++;
     }
+
+    // paranoid
+    assert(var_remap.size() == reg_vars.second.order.size());
+    std::unordered_set<int> check;
+    for (auto kv : var_remap) {
+      check.insert(kv.second);
+    }
+    assert(check.size() == var_remap.size());
+
     map.remap_reg(reg_vars.first, var_remap);
     program_read_vars[reg_vars.first].resize(i);
     program_write_vars[reg_vars.first].resize(i);
@@ -554,6 +585,9 @@ void update_var_info(VariableNames::VarInfo* info,
 }
 }  // namespace
 
+/*!
+ * Create variable info for each variable.
+ */
 void SSA::make_vars(const Function& function, const DecompilerTypeSystem& dts) {
   for (int block_id = 0; block_id < int(blocks.size()); block_id++) {
     const auto& block = blocks.at(block_id);
@@ -595,7 +629,7 @@ void remap_color_move(
   old_kv->second.at(old_var.id).reg_id = new_var;
 }
 
-VariableNames SSA::get_vars() {
+VariableNames SSA::get_vars() const {
   VariableNames result;
   result.read_vars = program_read_vars;
   result.write_vars = program_write_vars;
@@ -646,6 +680,71 @@ VariableNames SSA::get_vars() {
   return result;
 }
 
+/*!
+ * Get a map from access to SSA variable.
+ */
+RegAccessMap<int> SSA::get_ssa_mapping() {
+  RegAccessMap<int> result;
+
+  for (const auto& block : blocks) {
+    for (const auto& instr : block.ins) {
+      if (instr.dst.has_value()) {
+        RegisterAccess access(AccessMode::WRITE, instr.dst->reg(), instr.op_id, true);
+        result[access] = map.var_id(*instr.dst);
+      }
+
+      for (const auto& src : instr.src) {
+        RegisterAccess access(AccessMode::READ, src.reg(), instr.op_id, true);
+        result[access] = map.var_id(src);
+      }
+    }
+  }
+
+  return result;
+}
+
+/*!
+ * Find Program Variables that can safely be propagated.
+ */
+std::unordered_map<RegId, UseDefInfo, RegId::hash> SSA::get_use_def_info(
+    const RegAccessMap<int>& ssa_info) const {
+  std::unordered_map<RegId, UseDefInfo, RegId::hash> result;
+
+  // now, iterate through instruction
+  // for (const auto& block : blocks) {
+  for (size_t block_id = 0; block_id < blocks.size(); block_id++) {
+    const auto& block = blocks[block_id];
+    for (const auto& instr : block.ins) {
+      if (instr.is_dead_set) {
+        continue;
+      }
+      if (instr.dst.has_value()) {
+        // get the SSA var:
+        auto ssa_var_id =
+            ssa_info.at(RegisterAccess(AccessMode::WRITE, instr.dst->reg(), instr.op_id, true));
+        // get the info
+        auto& info = result[RegId(instr.dst->reg(), map.var_id(*instr.dst))];
+        // remember which SSA variable was in use here
+        info.defs.push_back({instr.op_id, (int)block_id, AccessMode::WRITE});
+        info.ssa_vars.insert(ssa_var_id);
+      }
+
+      for (const auto& src : instr.src) {
+        // get the SSA var:
+        auto ssa_var_id =
+            ssa_info.at(RegisterAccess(AccessMode::READ, src.reg(), instr.op_id, true));
+        // get the info
+        auto& info = result[RegId(src.reg(), map.var_id(src))];
+        // remember the variable
+        info.ssa_vars.insert(ssa_var_id);
+        info.uses.push_back({instr.op_id, (int)block_id, AccessMode::READ});
+      }
+    }
+  }
+
+  return result;
+}
+
 std::optional<VariableNames> run_variable_renaming(const Function& function,
                                                    const RegUsageInfo& rui,
                                                    const FunctionAtomicOps& ops,
@@ -694,14 +793,17 @@ std::optional<VariableNames> run_variable_renaming(const Function& function,
     fmt::print("Basic SSA\n{}\n------------------------------------\n", ssa.print());
   }
 
-  // eliminate PHIs that are stupid.
+  // eliminate PHIs that are not needed, still keeping us in SSA.
   while (ssa.simplify()) {
   }
   if (debug_prints) {
     fmt::print("Simplified SSA\n{}-------------------------------\n", ssa.print());
   }
 
-  // Merge phis to return to executable code.
+  // remember what the SSA mapping was:
+  auto ssa_mapping = ssa.get_ssa_mapping();
+
+  // Merge phis to return to executable code and exit SSA.
   if (debug_prints) {
     ssa.map.debug_print_map();
   }
@@ -725,7 +827,10 @@ std::optional<VariableNames> run_variable_renaming(const Function& function,
   if (function.ir2.env.has_type_analysis()) {
     // make vars
     ssa.make_vars(function, dts);
-    return ssa.get_vars();
+    //
+    auto result = ssa.get_vars();
+    result.use_def_info = ssa.get_use_def_info(ssa_mapping);
+    return result;
   } else {
     return std::nullopt;
   }

decompiler/analysis/variable_naming.h

@@ -36,6 +36,8 @@ struct FunctionAtomicOps;
  * These must be created from a VarMapSSA, which can then remap and merge these.
  * This remapping/merging functionality is used in the initial conversion to SSA,
  * the simplification of the SSA, and the merging of variables.
+ *
+ * Note - these are like references to SSA or program variable.
  */
 class VarSSA {
  public:
@@ -72,7 +74,7 @@ class VarMapSSA {
   void merge_to_first(const VarSSA& var_a, const VarSSA& var_b);
   std::string to_string(const VarSSA& var) const;
   bool same(const VarSSA& var_a, const VarSSA& var_b) const;
-  int var_id(const VarSSA& var);
+  int var_id(const VarSSA& var) const;
   void remap_reg(Register reg, const std::unordered_map<int, int>& remap);
   void debug_print_map() const;
 
@@ -109,6 +111,7 @@ struct SSA {
     std::vector<VarSSA> src;
     int op_id = -1;
     bool is_arg_coloring_move = false;
+    bool is_dead_set = false;
 
     std::string print(const VarMapSSA& var_map) const;
   };
@@ -134,11 +137,15 @@ struct SSA {
   VarSSA get_phi_dest(int block, Register dest_reg);
   void add_source_to_phi(int block, Register dest_reg, const VarSSA& src_var);
 
+  RegAccessMap<int> get_ssa_mapping();
+
   bool simplify();
   void merge_all_phis();
   void remap();
   void make_vars(const Function& function, const DecompilerTypeSystem& dts);
-  VariableNames get_vars();
+  std::unordered_map<RegId, UseDefInfo, RegId::hash> get_use_def_info(
+      const RegAccessMap<int>& ssa_info) const;
+  VariableNames get_vars() const;
   std::string print() const;
 };