Extracted block merging functionality into its own utility file (#2325)

* Extracted useful functionality from block merger and exposed it as stand-alone methods.

* Separated these methods into a utility file.

Android.mk

@@ -76,6 +76,7 @@ SPVTOOLS_OPT_SRC_FILES := \
 		source/opt/aggressive_dead_code_elim_pass.cpp \
 		source/opt/basic_block.cpp \
 		source/opt/block_merge_pass.cpp \
+		source/opt/block_merge_util.cpp \
 		source/opt/build_module.cpp \
 		source/opt/cfg.cpp \
 		source/opt/cfg_cleanup_pass.cpp \

source/opt/CMakeLists.txt

@@ -15,6 +15,7 @@ set(SPIRV_TOOLS_OPT_SOURCES
   aggressive_dead_code_elim_pass.h
   basic_block.h
   block_merge_pass.h
+  block_merge_util.h
   build_module.h
   ccp_pass.h
   cfg_cleanup_pass.h
@@ -108,6 +109,7 @@ set(SPIRV_TOOLS_OPT_SOURCES
   aggressive_dead_code_elim_pass.cpp
   basic_block.cpp
   block_merge_pass.cpp
+  block_merge_util.cpp
   build_module.cpp
   ccp_pass.cpp
   cfg_cleanup_pass.cpp

source/opt/block_merge_pass.cpp

@@ -18,6 +18,7 @@
 
 #include <vector>
 
+#include "source/opt/block_merge_util.h"
 #include "source/opt/ir_context.h"
 #include "source/opt/iterator.h"
 
@@ -27,112 +28,17 @@ namespace opt {
 bool BlockMergePass::MergeBlocks(Function* func) {
   bool modified = false;
   for (auto bi = func->begin(); bi != func->end();) {
-    // Find block with single successor which has no other predecessors.
-    auto ii = bi->end();
-    --ii;
-    Instruction* br = &*ii;
-    if (br->opcode() != SpvOpBranch) {
+    if (blockmergeutil::CanMergeWithSuccessor(context(), &*bi)) {
+      blockmergeutil::MergeWithSuccessor(context(), func, bi);
+      // Reprocess block.
+      modified = true;
+    } else {
       ++bi;
-      continue;
     }
-
-    const uint32_t lab_id = br->GetSingleWordInOperand(0);
-    if (cfg()->preds(lab_id).size() != 1) {
-      ++bi;
-      continue;
-    }
-
-    bool pred_is_merge = IsMerge(&*bi);
-    bool succ_is_merge = IsMerge(lab_id);
-    if (pred_is_merge && succ_is_merge) {
-      // Cannot merge two merges together.
-      ++bi;
-      continue;
-    }
-
-    Instruction* merge_inst = bi->GetMergeInst();
-    bool pred_is_header = IsHeader(&*bi);
-    if (pred_is_header && lab_id != merge_inst->GetSingleWordInOperand(0u)) {
-      bool succ_is_header = IsHeader(lab_id);
-      if (pred_is_header && succ_is_header) {
-        // Cannot merge two headers together when the successor is not the merge
-        // block of the predecessor.
-        ++bi;
-        continue;
-      }
-
-      // If this is a header block and the successor is not its merge, we must
-      // be careful about which blocks we are willing to merge together.
-      // OpLoopMerge must be followed by a conditional or unconditional branch.
-      // The merge must be a loop merge because a selection merge cannot be
-      // followed by an unconditional branch.
-      BasicBlock* succ_block = context()->get_instr_block(lab_id);
-      SpvOp succ_term_op = succ_block->terminator()->opcode();
-      assert(merge_inst->opcode() == SpvOpLoopMerge);
-      if (succ_term_op != SpvOpBranch &&
-          succ_term_op != SpvOpBranchConditional) {
-        ++bi;
-        continue;
-      }
-    }
-
-    // Merge blocks.
-    context()->KillInst(br);
-    auto sbi = bi;
-    for (; sbi != func->end(); ++sbi)
-      if (sbi->id() == lab_id) break;
-    // If bi is sbi's only predecessor, it dominates sbi and thus
-    // sbi must follow bi in func's ordering.
-    assert(sbi != func->end());
-
-    // Update the inst-to-block mapping for the instructions in sbi.
-    for (auto& inst : *sbi) {
-      context()->set_instr_block(&inst, &*bi);
-    }
-
-    // Now actually move the instructions.
-    bi->AddInstructions(&*sbi);
-
-    if (merge_inst) {
-      if (pred_is_header && lab_id == merge_inst->GetSingleWordInOperand(0u)) {
-        // Merging the header and merge blocks, so remove the structured control
-        // flow declaration.
-        context()->KillInst(merge_inst);
-      } else {
-        // Move the merge instruction to just before the terminator.
-        merge_inst->InsertBefore(bi->terminator());
-      }
-    }
-    context()->ReplaceAllUsesWith(lab_id, bi->id());
-    context()->KillInst(sbi->GetLabelInst());
-    (void)sbi.Erase();
-    // Reprocess block.
-    modified = true;
   }
   return modified;
 }
 
-bool BlockMergePass::IsHeader(BasicBlock* block) {
-  return block->GetMergeInst() != nullptr;
-}
-
-bool BlockMergePass::IsHeader(uint32_t id) {
-  return IsHeader(context()->get_instr_block(get_def_use_mgr()->GetDef(id)));
-}
-
-bool BlockMergePass::IsMerge(uint32_t id) {
-  return !get_def_use_mgr()->WhileEachUse(id, [](Instruction* user,
-                                                 uint32_t index) {
-    SpvOp op = user->opcode();
-    if ((op == SpvOpLoopMerge || op == SpvOpSelectionMerge) && index == 0u) {
-      return false;
-    }
-    return true;
-  });
-}
-
-bool BlockMergePass::IsMerge(BasicBlock* block) { return IsMerge(block->id()); }
-
 Pass::Status BlockMergePass::Process() {
   // Process all entry point functions.
   ProcessFunction pfn = [this](Function* fp) { return MergeBlocks(fp); };

source/opt/block_merge_pass.h

@@ -54,14 +54,6 @@ class BlockMergePass : public Pass {
   // with no other predecessors. Merge these blocks into a single block.
   bool MergeBlocks(Function* func);
 
-  // Returns true if |block| (or |id|) contains a merge instruction.
-  bool IsHeader(BasicBlock* block);
-  bool IsHeader(uint32_t id);
-
-  // Returns true if |block| (or |id|) is the merge target of a merge
-  // instruction.
-  bool IsMerge(BasicBlock* block);
-  bool IsMerge(uint32_t id);
 };
 
 }  // namespace opt

source/opt/block_merge_util.cpp

@@ -0,0 +1,147 @@
+// Copyright (c) 2017 The Khronos Group Inc.
+// Copyright (c) 2017 Valve Corporation
+// Copyright (c) 2017 LunarG Inc.
+// Copyright (c) 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "block_merge_util.h"
+
+namespace spvtools {
+namespace opt {
+namespace blockmergeutil {
+
+namespace {
+
+// Returns true if |block| contains a merge instruction.
+bool IsHeader(BasicBlock* block) { return block->GetMergeInst() != nullptr; }
+
+// Returns true if |id| contains a merge instruction.
+bool IsHeader(IRContext* context, uint32_t id) {
+  return IsHeader(
+      context->get_instr_block(context->get_def_use_mgr()->GetDef(id)));
+}
+
+// Returns true if |id| is the merge target of a merge instruction.
+bool IsMerge(IRContext* context, uint32_t id) {
+  return !context->get_def_use_mgr()->WhileEachUse(id, [](Instruction* user,
+                                                          uint32_t index) {
+    SpvOp op = user->opcode();
+    if ((op == SpvOpLoopMerge || op == SpvOpSelectionMerge) && index == 0u) {
+      return false;
+    }
+    return true;
+  });
+}
+
+// Returns true if |block| is the merge target of a merge instruction.
+bool IsMerge(IRContext* context, BasicBlock* block) {
+  return IsMerge(context, block->id());
+}
+
+}  // Anonymous namespace
+
+bool CanMergeWithSuccessor(IRContext* context, BasicBlock* block) {
+  // Find block with single successor which has no other predecessors.
+  auto ii = block->end();
+  --ii;
+  Instruction* br = &*ii;
+  if (br->opcode() != SpvOpBranch) {
+    return false;
+  }
+
+  const uint32_t lab_id = br->GetSingleWordInOperand(0);
+  if (context->cfg()->preds(lab_id).size() != 1) {
+    return false;
+  }
+
+  bool pred_is_merge = IsMerge(context, block);
+  bool succ_is_merge = IsMerge(context, lab_id);
+  if (pred_is_merge && succ_is_merge) {
+    // Cannot merge two merges together.
+    return false;
+  }
+
+  Instruction* merge_inst = block->GetMergeInst();
+  const bool pred_is_header = IsHeader(block);
+  if (pred_is_header && lab_id != merge_inst->GetSingleWordInOperand(0u)) {
+    bool succ_is_header = IsHeader(context, lab_id);
+    if (pred_is_header && succ_is_header) {
+      // Cannot merge two headers together when the successor is not the merge
+      // block of the predecessor.
+      return false;
+    }
+
+    // If this is a header block and the successor is not its merge, we must
+    // be careful about which blocks we are willing to merge together.
+    // OpLoopMerge must be followed by a conditional or unconditional branch.
+    // The merge must be a loop merge because a selection merge cannot be
+    // followed by an unconditional branch.
+    BasicBlock* succ_block = context->get_instr_block(lab_id);
+    SpvOp succ_term_op = succ_block->terminator()->opcode();
+    assert(merge_inst->opcode() == SpvOpLoopMerge);
+    if (succ_term_op != SpvOpBranch && succ_term_op != SpvOpBranchConditional) {
+      return false;
+    }
+  }
+  return true;
+}
+
+void MergeWithSuccessor(IRContext* context, Function* func,
+                        Function::iterator bi) {
+  assert(CanMergeWithSuccessor(context, &*bi) &&
+         "Precondition failure for MergeWithSuccessor: it must be legal to "
+         "merge the block and its successor.");
+
+  auto ii = bi->end();
+  --ii;
+  Instruction* br = &*ii;
+  const uint32_t lab_id = br->GetSingleWordInOperand(0);
+  Instruction* merge_inst = bi->GetMergeInst();
+  bool pred_is_header = IsHeader(&*bi);
+
+  // Merge blocks.
+  context->KillInst(br);
+  auto sbi = bi;
+  for (; sbi != func->end(); ++sbi)
+    if (sbi->id() == lab_id) break;
+  // If bi is sbi's only predecessor, it dominates sbi and thus
+  // sbi must follow bi in func's ordering.
+  assert(sbi != func->end());
+
+  // Update the inst-to-block mapping for the instructions in sbi.
+  for (auto& inst : *sbi) {
+    context->set_instr_block(&inst, &*bi);
+  }
+
+  // Now actually move the instructions.
+  bi->AddInstructions(&*sbi);
+
+  if (merge_inst) {
+    if (pred_is_header && lab_id == merge_inst->GetSingleWordInOperand(0u)) {
+      // Merging the header and merge blocks, so remove the structured control
+      // flow declaration.
+      context->KillInst(merge_inst);
+    } else {
+      // Move the merge instruction to just before the terminator.
+      merge_inst->InsertBefore(bi->terminator());
+    }
+  }
+  context->ReplaceAllUsesWith(lab_id, bi->id());
+  context->KillInst(sbi->GetLabelInst());
+  (void)sbi.Erase();
+}
+
+}  // namespace blockmergeutil
+}  // namespace opt
+}  // namespace spvtools

source/opt/block_merge_util.h

@@ -0,0 +1,44 @@
+// Copyright (c) 2017 The Khronos Group Inc.
+// Copyright (c) 2017 Valve Corporation
+// Copyright (c) 2017 LunarG Inc.
+// Copyright (c) 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef SOURCE_OPT_BLOCK_MERGE_UTIL_H_
+#define SOURCE_OPT_BLOCK_MERGE_UTIL_H_
+
+#include "source/opt/ir_context.h"
+
+namespace spvtools {
+namespace opt {
+
+// Provides functions for determining when it is safe to merge blocks, and for
+// actually merging blocks, for use by various analyses and passes.
+namespace blockmergeutil {
+
+// Returns true if and only if |block| has exactly one successor and merging
+// this successor into |block| has no impact on the semantics or validity of the
+// SPIR-V module.
+bool CanMergeWithSuccessor(IRContext* context, BasicBlock* block);
+
+// Requires that |bi| has a successor that can be safely merged into |bi|, and
+// performs the merge.
+void MergeWithSuccessor(IRContext* context, Function* func,
+                        Function::iterator bi);
+
+}  // namespace blockmergeutil
+}  // namespace opt
+}  // namespace spvtools
+
+#endif  //  SOURCE_OPT_BLOCK_MERGE_UTIL_H_