spirv-reduce: Refactoring (#3793)

Avoids the use of "using" in favour of explicit qualification, to be consistent with spirv-fuzz. Fixes indentation in a TODO comment. Addresses and removes two existing TODO comments by moving some helper functionality into reduction_util. Related issue: #2184.
2020-09-10 22:03:40 +01:00 · 2020-09-10 22:03:40 +01:00 · de7d57984d
--- a/source/reduce/conditional_branch_to_simple_conditional_branch_opportunity_finder.cpp
+++ b/source/reduce/conditional_branch_to_simple_conditional_branch_opportunity_finder.cpp
@ -20,12 +20,9 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-using opt::Instruction;
-
 std::vector<std::unique_ptr<ReductionOpportunity>>
 ConditionalBranchToSimpleConditionalBranchOpportunityFinder::
-    GetAvailableOpportunities(IRContext* context) const {
+    GetAvailableOpportunities(opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  // Find the opportunities for redirecting all false targets before the
@ -39,7 +36,7 @@ ConditionalBranchToSimpleConditionalBranchOpportunityFinder::
      // Consider every block in the function.
      for (auto& block : function) {
        // The terminator must be SpvOpBranchConditional.
-        Instruction* terminator = block.terminator();
+        opt::Instruction* terminator = block.terminator();
        if (terminator->opcode() != SpvOpBranchConditional) {
          continue;
        }
--- a/source/reduce/conditional_branch_to_simple_conditional_branch_reduction_opportunity.cpp
+++ b/source/reduce/conditional_branch_to_simple_conditional_branch_reduction_opportunity.cpp
@ -19,13 +19,10 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-using opt::Instruction;
-
 ConditionalBranchToSimpleConditionalBranchReductionOpportunity::
    ConditionalBranchToSimpleConditionalBranchReductionOpportunity(
-        IRContext* context, Instruction* conditional_branch_instruction,
-        bool redirect_to_true)
+        opt::IRContext* context,
+        opt::Instruction* conditional_branch_instruction, bool redirect_to_true)
    : context_(context),
      conditional_branch_instruction_(conditional_branch_instruction),
      redirect_to_true_(redirect_to_true) {}
@ -63,7 +60,8 @@ void ConditionalBranchToSimpleConditionalBranchReductionOpportunity::Apply() {
      context_->cfg()->block(old_successor_block_id));

  // We have changed the CFG.
-  context_->InvalidateAnalysesExceptFor(IRContext::Analysis::kAnalysisNone);
+  context_->InvalidateAnalysesExceptFor(
+      opt::IRContext::Analysis::kAnalysisNone);
 }

 }  // namespace reduce
--- a/source/reduce/merge_blocks_reduction_opportunity.cpp
+++ b/source/reduce/merge_blocks_reduction_opportunity.cpp
@ -20,12 +20,8 @@
 namespace spvtools {
 namespace reduce {

-using opt::BasicBlock;
-using opt::Function;
-using opt::IRContext;
-
 MergeBlocksReductionOpportunity::MergeBlocksReductionOpportunity(
-    IRContext* context, Function* function, BasicBlock* block) {
+    opt::IRContext* context, opt::Function* function, opt::BasicBlock* block) {
  // Precondition: the terminator has to be OpBranch.
  assert(block->terminator()->opcode() == SpvOpBranch);
  context_ = context;
@ -49,7 +45,8 @@ bool MergeBlocksReductionOpportunity::PreconditionHolds() {
         "For a successor to be merged into its predecessor, exactly one "
         "predecessor must be present.");
  const uint32_t predecessor_id = predecessors[0];
-  BasicBlock* predecessor_block = context_->get_instr_block(predecessor_id);
+  opt::BasicBlock* predecessor_block =
+      context_->get_instr_block(predecessor_id);
  return opt::blockmergeutil::CanMergeWithSuccessor(context_,
                                                    predecessor_block);
 }
@ -70,7 +67,8 @@ void MergeBlocksReductionOpportunity::Apply() {
    if (bi->id() == predecessor_id) {
      opt::blockmergeutil::MergeWithSuccessor(context_, function_, bi);
      // Block merging changes the control flow graph, so invalidate it.
-      context_->InvalidateAnalysesExceptFor(IRContext::Analysis::kAnalysisNone);
+      context_->InvalidateAnalysesExceptFor(
+          opt::IRContext::Analysis::kAnalysisNone);
      return;
    }
  }
--- a/source/reduce/merge_blocks_reduction_opportunity_finder.cpp
+++ b/source/reduce/merge_blocks_reduction_opportunity_finder.cpp
@ -19,15 +19,13 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-
 std::string MergeBlocksReductionOpportunityFinder::GetName() const {
  return "MergeBlocksReductionOpportunityFinder";
 }

 std::vector<std::unique_ptr<ReductionOpportunity>>
 MergeBlocksReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  // Consider every block in every function.
--- a/source/reduce/operand_to_const_reduction_opportunity_finder.cpp
+++ b/source/reduce/operand_to_const_reduction_opportunity_finder.cpp
@ -20,11 +20,9 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-
 std::vector<std::unique_ptr<ReductionOpportunity>>
 OperandToConstReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;
  assert(result.empty());

--- a/source/reduce/operand_to_dominating_id_reduction_opportunity_finder.cpp
+++ b/source/reduce/operand_to_dominating_id_reduction_opportunity_finder.cpp
@ -20,13 +20,9 @@
 namespace spvtools {
 namespace reduce {

-using opt::Function;
-using opt::IRContext;
-using opt::Instruction;
-
 std::vector<std::unique_ptr<ReductionOpportunity>>
 OperandToDominatingIdReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  // Go through every instruction in every block, considering it as a potential
@ -61,9 +57,9 @@ OperandToDominatingIdReductionOpportunityFinder::GetAvailableOpportunities(
 void OperandToDominatingIdReductionOpportunityFinder::
    GetOpportunitiesForDominatingInst(
        std::vector<std::unique_ptr<ReductionOpportunity>>* opportunities,
-        Instruction* candidate_dominator,
-        Function::iterator candidate_dominator_block, Function* function,
-        IRContext* context) const {
+        opt::Instruction* candidate_dominator,
+        opt::Function::iterator candidate_dominator_block,
+        opt::Function* function, opt::IRContext* context) const {
  assert(candidate_dominator->HasResultId());
  assert(candidate_dominator->type_id());
  auto dominator_analysis = context->GetDominatorAnalysis(function);
--- a/source/reduce/operand_to_undef_reduction_opportunity_finder.cpp
+++ b/source/reduce/operand_to_undef_reduction_opportunity_finder.cpp
@ -20,11 +20,9 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-
 std::vector<std::unique_ptr<ReductionOpportunity>>
 OperandToUndefReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  for (auto& function : *context->module()) {
--- a/source/reduce/reduction_util.cpp
+++ b/source/reduce/reduction_util.cpp
@ -15,17 +15,73 @@
 #include "source/reduce/reduction_util.h"

 #include "source/opt/ir_context.h"
+#include "source/util/make_unique.h"

 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-using opt::Instruction;
-
 const uint32_t kTrueBranchOperandIndex = 1;
 const uint32_t kFalseBranchOperandIndex = 2;

-uint32_t FindOrCreateGlobalUndef(IRContext* context, uint32_t type_id) {
+uint32_t FindOrCreateGlobalVariable(opt::IRContext* context,
+                                    uint32_t pointer_type_id) {
+  for (auto& inst : context->module()->types_values()) {
+    if (inst.opcode() != SpvOpVariable) {
+      continue;
+    }
+    if (inst.type_id() == pointer_type_id) {
+      return inst.result_id();
+    }
+  }
+  const uint32_t variable_id = context->TakeNextId();
+  auto variable_inst = MakeUnique<opt::Instruction>(
+      context, SpvOpVariable, pointer_type_id, variable_id,
+      opt::Instruction::OperandList(
+          {{SPV_OPERAND_TYPE_STORAGE_CLASS,
+            {static_cast<uint32_t>(context->get_type_mgr()
+                                       ->GetType(pointer_type_id)
+                                       ->AsPointer()
+                                       ->storage_class())}}}));
+  context->module()->AddGlobalValue(std::move(variable_inst));
+  return variable_id;
+}
+
+uint32_t FindOrCreateFunctionVariable(opt::IRContext* context,
+                                      opt::Function* function,
+                                      uint32_t pointer_type_id) {
+  // The pointer type of a function variable must have Function storage class.
+  assert(context->get_type_mgr()
+             ->GetType(pointer_type_id)
+             ->AsPointer()
+             ->storage_class() == SpvStorageClassFunction);
+
+  // Go through the instructions in the function's first block until we find a
+  // suitable variable, or go past all the variables.
+  opt::BasicBlock::iterator iter = function->begin()->begin();
+  for (;; ++iter) {
+    // We will either find a suitable variable, or find a non-variable
+    // instruction; we won't exhaust all instructions.
+    assert(iter != function->begin()->end());
+    if (iter->opcode() != SpvOpVariable) {
+      // If we see a non-variable, we have gone through all the variables.
+      break;
+    }
+    if (iter->type_id() == pointer_type_id) {
+      return iter->result_id();
+    }
+  }
+  // At this point, iter refers to the first non-function instruction of the
+  // function's entry block.
+  const uint32_t variable_id = context->TakeNextId();
+  auto variable_inst = MakeUnique<opt::Instruction>(
+      context, SpvOpVariable, pointer_type_id, variable_id,
+      opt::Instruction::OperandList(
+          {{SPV_OPERAND_TYPE_STORAGE_CLASS, {SpvStorageClassFunction}}}));
+  iter->InsertBefore(std::move(variable_inst));
+  return variable_id;
+}
+
+uint32_t FindOrCreateGlobalUndef(opt::IRContext* context, uint32_t type_id) {
  for (auto& inst : context->module()->types_values()) {
    if (inst.opcode() != SpvOpUndef) {
      continue;
@ -34,11 +90,9 @@ uint32_t FindOrCreateGlobalUndef(IRContext* context, uint32_t type_id) {
      return inst.result_id();
    }
  }
-  // TODO(2182): this is adapted from MemPass::Type2Undef.  In due course it
-  // would be good to factor out this duplication.
  const uint32_t undef_id = context->TakeNextId();
-  std::unique_ptr<Instruction> undef_inst(
-      new Instruction(context, SpvOpUndef, type_id, undef_id, {}));
+  auto undef_inst = MakeUnique<opt::Instruction>(
+      context, SpvOpUndef, type_id, undef_id, opt::Instruction::OperandList());
  assert(undef_id == undef_inst->result_id());
  context->module()->AddGlobalValue(std::move(undef_inst));
  return undef_id;
@ -46,8 +100,8 @@ uint32_t FindOrCreateGlobalUndef(IRContext* context, uint32_t type_id) {

 void AdaptPhiInstructionsForRemovedEdge(uint32_t from_id,
                                        opt::BasicBlock* to_block) {
-  to_block->ForEachPhiInst([&from_id](Instruction* phi_inst) {
-    Instruction::OperandList new_in_operands;
+  to_block->ForEachPhiInst([&from_id](opt::Instruction* phi_inst) {
+    opt::Instruction::OperandList new_in_operands;
    // Go through the OpPhi's input operands in (variable, parent) pairs.
    for (uint32_t index = 0; index < phi_inst->NumInOperands(); index += 2) {
      // Keep all pairs where the parent is not the block from which the edge
--- a/source/reduce/reduction_util.h
+++ b/source/reduce/reduction_util.h
@ -26,6 +26,16 @@ namespace reduce {
 extern const uint32_t kTrueBranchOperandIndex;
 extern const uint32_t kFalseBranchOperandIndex;

+// Returns a global OpVariable of type |pointer_type_id|, adding one if none
+// exist.
+uint32_t FindOrCreateGlobalVariable(opt::IRContext* context,
+                                    uint32_t pointer_type_id);
+
+// Returns an OpVariable of type |pointer_type_id| declared in |function|,
+// adding one if none exist.
+uint32_t FindOrCreateFunctionVariable(opt::IRContext* context, opt::Function*,
+                                      uint32_t pointer_type_id);
+
 // Returns an OpUndef id from the global value list that is of the given type,
 // adding one if it does not exist.
 uint32_t FindOrCreateGlobalUndef(opt::IRContext* context, uint32_t type_id);
--- a/source/reduce/remove_block_reduction_opportunity.cpp
+++ b/source/reduce/remove_block_reduction_opportunity.cpp
@ -19,11 +19,8 @@
 namespace spvtools {
 namespace reduce {

-using opt::BasicBlock;
-using opt::Function;
-
 RemoveBlockReductionOpportunity::RemoveBlockReductionOpportunity(
-    Function* function, BasicBlock* block)
+    opt::Function* function, opt::BasicBlock* block)
    : function_(function), block_(block) {
  // precondition:
  assert(block_->begin() != block_->end() &&
--- a/source/reduce/remove_block_reduction_opportunity_finder.cpp
+++ b/source/reduce/remove_block_reduction_opportunity_finder.cpp
@ -19,17 +19,13 @@
 namespace spvtools {
 namespace reduce {

-using opt::Function;
-using opt::IRContext;
-using opt::Instruction;
-
 std::string RemoveBlockReductionOpportunityFinder::GetName() const {
  return "RemoveBlockReductionOpportunityFinder";
 }

 std::vector<std::unique_ptr<ReductionOpportunity>>
 RemoveBlockReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  // Consider every block in every function.
@ -45,7 +41,8 @@ RemoveBlockReductionOpportunityFinder::GetAvailableOpportunities(
 }

 bool RemoveBlockReductionOpportunityFinder::IsBlockValidOpportunity(
-    IRContext* context, Function& function, Function::iterator& bi) {
+    opt::IRContext* context, opt::Function& function,
+    opt::Function::iterator& bi) {
  assert(bi != function.end() && "Block iterator was out of bounds");

  // Don't remove first block; we don't want to end up with no blocks.
@ -67,19 +64,19 @@ bool RemoveBlockReductionOpportunityFinder::IsBlockValidOpportunity(
 }

 bool RemoveBlockReductionOpportunityFinder::
-    BlockInstructionsHaveNoOutsideReferences(IRContext* context,
-                                             const Function::iterator& bi) {
+    BlockInstructionsHaveNoOutsideReferences(
+        opt::IRContext* context, const opt::Function::iterator& bi) {
  // Get all instructions in block.
  std::unordered_set<uint32_t> instructions_in_block;
-  for (const Instruction& instruction : *bi) {
+  for (const opt::Instruction& instruction : *bi) {
    instructions_in_block.insert(instruction.unique_id());
  }

  // For each instruction...
-  for (const Instruction& instruction : *bi) {
+  for (const opt::Instruction& instruction : *bi) {
    // For each use of the instruction...
    bool no_uses_outside_block = context->get_def_use_mgr()->WhileEachUser(
-        &instruction, [&instructions_in_block](Instruction* user) -> bool {
+        &instruction, [&instructions_in_block](opt::Instruction* user) -> bool {
          // If the use is in this block, continue (return true). Otherwise, we
          // found an outside use; return false (and stop).
          return instructions_in_block.find(user->unique_id()) !=
--- a/source/reduce/remove_selection_reduction_opportunity_finder.cpp
+++ b/source/reduce/remove_selection_reduction_opportunity_finder.cpp
@ -19,10 +19,6 @@
 namespace spvtools {
 namespace reduce {

-using opt::BasicBlock;
-using opt::IRContext;
-using opt::Instruction;
-
 namespace {
 const uint32_t kMergeNodeIndex = 0;
 const uint32_t kContinueNodeIndex = 1;
@ -34,7 +30,7 @@ std::string RemoveSelectionReductionOpportunityFinder::GetName() const {

 std::vector<std::unique_ptr<ReductionOpportunity>>
 RemoveSelectionReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  // Get all loop merge and continue blocks so we can check for these later.
  std::unordered_set<uint32_t> merge_and_continue_blocks_from_loops;
  for (auto& function : *context->module()) {
@ -73,8 +69,8 @@ RemoveSelectionReductionOpportunityFinder::GetAvailableOpportunities(
 }

 bool RemoveSelectionReductionOpportunityFinder::CanOpSelectionMergeBeRemoved(
-    IRContext* context, const BasicBlock& header_block,
-    Instruction* merge_instruction,
+    opt::IRContext* context, const opt::BasicBlock& header_block,
+    opt::Instruction* merge_instruction,
    std::unordered_set<uint32_t> merge_and_continue_blocks_from_loops) {
  assert(header_block.GetMergeInst() == merge_instruction &&
         "CanOpSelectionMergeBeRemoved(...): header block and merge "
@ -122,7 +118,7 @@ bool RemoveSelectionReductionOpportunityFinder::CanOpSelectionMergeBeRemoved(
        merge_instruction->GetSingleWordOperand(kMergeNodeIndex);
    for (uint32_t predecessor_block_id :
         context->cfg()->preds(merge_block_id)) {
-      const BasicBlock* predecessor_block =
+      const opt::BasicBlock* predecessor_block =
          context->cfg()->block(predecessor_block_id);
      assert(predecessor_block);
      bool found_divergent_successor = false;
--- a/source/reduce/simple_conditional_branch_to_branch_opportunity_finder.cpp
+++ b/source/reduce/simple_conditional_branch_to_branch_opportunity_finder.cpp
@ -20,12 +20,9 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-using opt::Instruction;
-
 std::vector<std::unique_ptr<ReductionOpportunity>>
 SimpleConditionalBranchToBranchOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  // Consider every function.
@ -33,7 +30,7 @@ SimpleConditionalBranchToBranchOpportunityFinder::GetAvailableOpportunities(
    // Consider every block in the function.
    for (auto& block : function) {
      // The terminator must be SpvOpBranchConditional.
-      Instruction* terminator = block.terminator();
+      opt::Instruction* terminator = block.terminator();
      if (terminator->opcode() != SpvOpBranchConditional) {
        continue;
      }
--- a/source/reduce/simple_conditional_branch_to_branch_reduction_opportunity.cpp
+++ b/source/reduce/simple_conditional_branch_to_branch_reduction_opportunity.cpp
@ -19,11 +19,9 @@
 namespace spvtools {
 namespace reduce {

-using namespace opt;
-
 SimpleConditionalBranchToBranchReductionOpportunity::
    SimpleConditionalBranchToBranchReductionOpportunity(
-        Instruction* conditional_branch_instruction)
+        opt::Instruction* conditional_branch_instruction)
    : conditional_branch_instruction_(conditional_branch_instruction) {}

 bool SimpleConditionalBranchToBranchReductionOpportunity::PreconditionHolds() {
--- a/source/reduce/structured_loop_to_selection_reduction_opportunity.cpp
+++ b/source/reduce/structured_loop_to_selection_reduction_opportunity.cpp
@ -21,11 +21,6 @@
 namespace spvtools {
 namespace reduce {

-using opt::BasicBlock;
-using opt::IRContext;
-using opt::Instruction;
-using opt::Operand;
-
 namespace {
 const uint32_t kMergeNodeIndex = 0;
 }  // namespace
@ -58,14 +53,16 @@ void StructuredLoopToSelectionReductionOpportunity::Apply() {

  // We have made control flow changes that do not preserve the analyses that
  // were performed.
-  context_->InvalidateAnalysesExceptFor(IRContext::Analysis::kAnalysisNone);
+  context_->InvalidateAnalysesExceptFor(
+      opt::IRContext::Analysis::kAnalysisNone);

  // (4) By changing CFG edges we may have created scenarios where ids are used
  // without being dominated; we fix instances of this.
  FixNonDominatedIdUses();

  // Invalidate the analyses we just used.
-  context_->InvalidateAnalysesExceptFor(IRContext::Analysis::kAnalysisNone);
+  context_->InvalidateAnalysesExceptFor(
+      opt::IRContext::Analysis::kAnalysisNone);
 }

 void StructuredLoopToSelectionReductionOpportunity::RedirectToClosestMergeBlock(
@ -168,13 +165,14 @@ void StructuredLoopToSelectionReductionOpportunity::RedirectEdge(
 }

 void StructuredLoopToSelectionReductionOpportunity::
-    AdaptPhiInstructionsForAddedEdge(uint32_t from_id, BasicBlock* to_block) {
-  to_block->ForEachPhiInst([this, &from_id](Instruction* phi_inst) {
+    AdaptPhiInstructionsForAddedEdge(uint32_t from_id,
+                                     opt::BasicBlock* to_block) {
+  to_block->ForEachPhiInst([this, &from_id](opt::Instruction* phi_inst) {
    // Add to the phi operand an (undef, from_id) pair to reflect the added
    // edge.
    auto undef_id = FindOrCreateGlobalUndef(context_, phi_inst->type_id());
-    phi_inst->AddOperand(Operand(SPV_OPERAND_TYPE_ID, {undef_id}));
-    phi_inst->AddOperand(Operand(SPV_OPERAND_TYPE_ID, {from_id}));
+    phi_inst->AddOperand(opt::Operand(SPV_OPERAND_TYPE_ID, {undef_id}));
+    phi_inst->AddOperand(opt::Operand(SPV_OPERAND_TYPE_ID, {from_id}));
  });
 }

@ -227,7 +225,7 @@ void StructuredLoopToSelectionReductionOpportunity::FixNonDominatedIdUses() {
        continue;
      }
      context_->get_def_use_mgr()->ForEachUse(&def, [this, &block, &def](
-                                                        Instruction* use,
+                                                        opt::Instruction* use,
                                                        uint32_t index) {
        // Ignore uses outside of blocks, such as in OpDecorate.
        if (context_->get_instr_block(use) == nullptr) {
@ -245,17 +243,20 @@ void StructuredLoopToSelectionReductionOpportunity::FixNonDominatedIdUses() {
              case SpvStorageClassFunction:
                use->SetOperand(
                    index, {FindOrCreateFunctionVariable(
+                               context_, enclosing_function_,
                               context_->get_type_mgr()->GetId(pointer_type))});
                break;
              default:
                // TODO(2183) Need to think carefully about whether it makes
-                // sense to add new variables for all storage classes; it's fine
-                // for Private but might not be OK for input/output storage
-                // classes for example.
+                //  sense to add new variables for all storage classes; it's
+                //  fine for Private but might not be OK for input/output
+                //  storage classes for example.
                use->SetOperand(
                    index, {FindOrCreateGlobalVariable(
+                               context_,
                               context_->get_type_mgr()->GetId(pointer_type))});
                break;
+                break;
            }
          } else {
            use->SetOperand(index,
@ -268,9 +269,10 @@ void StructuredLoopToSelectionReductionOpportunity::FixNonDominatedIdUses() {
 }

 bool StructuredLoopToSelectionReductionOpportunity::
-    DefinitionSufficientlyDominatesUse(Instruction* def, Instruction* use,
+    DefinitionSufficientlyDominatesUse(opt::Instruction* def,
+                                       opt::Instruction* use,
                                       uint32_t use_index,
-                                       BasicBlock& def_block) {
+                                       opt::BasicBlock& def_block) {
  if (use->opcode() == SpvOpPhi) {
    // A use in a phi doesn't need to be dominated by its definition, but the
    // associated parent block does need to be dominated by the definition.
@ -282,62 +284,5 @@ bool StructuredLoopToSelectionReductionOpportunity::
      ->Dominates(def, use);
 }

-uint32_t
-StructuredLoopToSelectionReductionOpportunity::FindOrCreateGlobalVariable(
-    uint32_t pointer_type_id) {
-  for (auto& inst : context_->module()->types_values()) {
-    if (inst.opcode() != SpvOpVariable) {
-      continue;
-    }
-    if (inst.type_id() == pointer_type_id) {
-      return inst.result_id();
-    }
-  }
-  const uint32_t variable_id = context_->TakeNextId();
-  std::unique_ptr<Instruction> variable_inst(
-      new Instruction(context_, SpvOpVariable, pointer_type_id, variable_id,
-                      {{SPV_OPERAND_TYPE_STORAGE_CLASS,
-                        {(uint32_t)context_->get_type_mgr()
-                             ->GetType(pointer_type_id)
-                             ->AsPointer()
-                             ->storage_class()}}}));
-  context_->module()->AddGlobalValue(std::move(variable_inst));
-  return variable_id;
-}
-
-uint32_t
-StructuredLoopToSelectionReductionOpportunity::FindOrCreateFunctionVariable(
-    uint32_t pointer_type_id) {
-  // The pointer type of a function variable must have Function storage class.
-  assert(context_->get_type_mgr()
-             ->GetType(pointer_type_id)
-             ->AsPointer()
-             ->storage_class() == SpvStorageClassFunction);
-
-  // Go through the instructions in the function's first block until we find a
-  // suitable variable, or go past all the variables.
-  BasicBlock::iterator iter = enclosing_function_->begin()->begin();
-  for (;; ++iter) {
-    // We will either find a suitable variable, or find a non-variable
-    // instruction; we won't exhaust all instructions.
-    assert(iter != enclosing_function_->begin()->end());
-    if (iter->opcode() != SpvOpVariable) {
-      // If we see a non-variable, we have gone through all the variables.
-      break;
-    }
-    if (iter->type_id() == pointer_type_id) {
-      return iter->result_id();
-    }
-  }
-  // At this point, iter refers to the first non-function instruction of the
-  // function's entry block.
-  const uint32_t variable_id = context_->TakeNextId();
-  std::unique_ptr<Instruction> variable_inst(new Instruction(
-      context_, SpvOpVariable, pointer_type_id, variable_id,
-      {{SPV_OPERAND_TYPE_STORAGE_CLASS, {SpvStorageClassFunction}}}));
-  iter->InsertBefore(std::move(variable_inst));
-  return variable_id;
-}
-
 }  // namespace reduce
 }  // namespace spvtools
--- a/source/reduce/structured_loop_to_selection_reduction_opportunity.h
+++ b/source/reduce/structured_loop_to_selection_reduction_opportunity.h
@ -86,20 +86,6 @@ class StructuredLoopToSelectionReductionOpportunity
                                          uint32_t use_index,
                                          opt::BasicBlock& def_block);

-  // Checks whether the global value list has an OpVariable of the given pointer
-  // type, adding one if not, and returns the id of such an OpVariable.
-  //
-  // TODO(2184): This will likely be used by other reduction passes, so should
-  // be factored out in due course.
-  uint32_t FindOrCreateGlobalVariable(uint32_t pointer_type_id);
-
-  // Checks whether the enclosing function has an OpVariable of the given
-  // pointer type, adding one if not, and returns the id of such an OpVariable.
-  //
-  // TODO(2184): This will likely be used by other reduction passes, so should
-  // be factored out in due course.
-  uint32_t FindOrCreateFunctionVariable(uint32_t pointer_type_id);
-
  opt::IRContext* context_;
  opt::BasicBlock* loop_construct_header_;
  opt::Function* enclosing_function_;
--- a/source/reduce/structured_loop_to_selection_reduction_opportunity_finder.cpp
+++ b/source/reduce/structured_loop_to_selection_reduction_opportunity_finder.cpp
@ -19,8 +19,6 @@
 namespace spvtools {
 namespace reduce {

-using opt::IRContext;
-
 namespace {
 const uint32_t kMergeNodeIndex = 0;
 const uint32_t kContinueNodeIndex = 1;
@ -28,7 +26,7 @@ const uint32_t kContinueNodeIndex = 1;

 std::vector<std::unique_ptr<ReductionOpportunity>>
 StructuredLoopToSelectionReductionOpportunityFinder::GetAvailableOpportunities(
-    IRContext* context) const {
+    opt::IRContext* context) const {
  std::vector<std::unique_ptr<ReductionOpportunity>> result;

  std::set<uint32_t> merge_block_ids;