Enhancements to block merging

* Should handle all possibilities
 * Stricter checks for what is disallowed:
  * header and header
  * merge and merge
 * Allow header and merge blocks to be merged
  * Erases the structured control declaration if merging header and
    merge blocks together.

diff --git a/source/opt/block_merge_pass.cpp b/source/opt/block_merge_pass.cpp
index c6a391e..a2bdf40 100644 (file)
--- a/source/opt/block_merge_pass.cpp
+++ b/source/opt/block_merge_pass.cpp
@@ -22,18 +22,6 @@
 namespace spvtools {
 namespace opt {
 
-bool BlockMergePass::HasMultipleRefs(uint32_t labId) {
-  bool multiple_refs = false;
-  return !get_def_use_mgr()->WhileEachUser(
-      labId, [&multiple_refs](ir::Instruction* user) {
-        if (user->opcode() != SpvOpName) {
-          if (multiple_refs) return false;
-          multiple_refs = true;
-        }
-        return true;
-      });
-}
-
 void BlockMergePass::KillInstAndName(ir::Instruction* inst) {
   std::vector<ir::Instruction*> to_kill;
   get_def_use_mgr()->ForEachUser(inst, [&to_kill](ir::Instruction* user) {
@@ -50,18 +38,7 @@ void BlockMergePass::KillInstAndName(ir::Instruction* inst) {
 bool BlockMergePass::MergeBlocks(ir::Function* func) {
   bool modified = false;
   for (auto bi = func->begin(); bi != func->end();) {
-    // Do not merge loop header blocks, at least for now.
-    if (bi->IsLoopHeader()) {
-      ++bi;
-      continue;
-    }
     // Find block with single successor which has no other predecessors.
-    // Continue and Merge blocks are currently ruled out as second blocks.
-    // Happily any such candidate blocks will have >1 uses due to their
-    // LoopMerge instruction.
-    // TODO(): Deal with phi instructions that reference the
-    // second block. Happily, these references currently inhibit
-    // the merge.
     auto ii = bi->end();
     --ii;
     ir::Instruction* br = &*ii;
@@ -69,28 +46,81 @@ bool BlockMergePass::MergeBlocks(ir::Function* func) {
       ++bi;
       continue;
     }
-    const uint32_t labId = br->GetSingleWordInOperand(0);
-    if (HasMultipleRefs(labId)) {
+
+    const uint32_t lab_id = br->GetSingleWordInOperand(0);
+    if (cfg()->preds(lab_id).size() != 1) {
+      ++bi;
+      continue;
+    }
+
+    bool pred_is_header = IsHeader(&*bi);
+    bool succ_is_header = IsHeader(lab_id);
+    if (pred_is_header && succ_is_header) {
+      // Cannot merge two headers together.
+      ++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;
     }
-    // Merge blocks
+
+    // Merge blocks.
+    ir::Instruction* merge_inst = bi->GetMergeInst();
     context()->KillInst(br);
     auto sbi = bi;
     for (; sbi != func->end(); ++sbi)
-      if (sbi->id() == labId) break;
+      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());
     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());
     KillInstAndName(sbi->GetLabelInst());
     (void)sbi.Erase();
-    // reprocess block
+    // Reprocess block.
     modified = true;
   }
   return modified;
 }
 
+bool BlockMergePass::IsHeader(ir::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, [](ir::Instruction* user,
+                                                 uint32_t index) {
+    SpvOp op = user->opcode();
+    if ((op == SpvOpLoopMerge || op == SpvOpSelectionMerge) && index == 0u) {
+      return false;
+    }
+    return true;
+  });
+}
+
+bool BlockMergePass::IsMerge(ir::BasicBlock* block) {
+  return IsMerge(block->id());
+}
+
 void BlockMergePass::Initialize(ir::IRContext* c) {
   InitializeProcessing(c);
 

diff --git a/source/opt/block_merge_pass.h b/source/opt/block_merge_pass.h
index 92921a5..d72259d 100644 (file)
--- a/source/opt/block_merge_pass.h
+++ b/source/opt/block_merge_pass.h
@@ -41,9 +41,6 @@ class BlockMergePass : public Pass {
   Status Process(ir::IRContext*) override;
 
  private:
-  // Return true if |labId| has multiple refs. Do not count OpName.
-  bool HasMultipleRefs(uint32_t labId);
-
   // Kill any OpName instruction referencing |inst|, then kill |inst|.
   void KillInstAndName(ir::Instruction* inst);
 
@@ -57,6 +54,15 @@ class BlockMergePass : public Pass {
   // Return true if all extensions in this module are allowed by this pass.
   bool AllExtensionsSupported() const;
 
+  // Returns true if |block| (or |id|) contains a merge instruction.
+  bool IsHeader(ir::BasicBlock* block);
+  bool IsHeader(uint32_t id);
+
+  // Returns true if |block| (or |id|) is the merge target of a merge
+  // instruction.
+  bool IsMerge(ir::BasicBlock* block);
+  bool IsMerge(uint32_t id);
+
   void Initialize(ir::IRContext* c);
   Pass::Status ProcessImpl();
 

diff --git a/test/opt/block_merge_test.cpp b/test/opt/block_merge_test.cpp
index de5e260..38a4969 100644 (file)
--- a/test/opt/block_merge_test.cpp
+++ b/test/opt/block_merge_test.cpp
@@ -158,63 +158,6 @@ OpFunctionEnd
                                              true, true);
 }
 
-TEST_F(BlockMergeTest, NoOptOfMergeOrContinueBlock) {
-  // Note: SPIR-V hand edited remove dead branch and add block
-  // before continue block
-  //
-  // #version 140
-  // in vec4 BaseColor;
-  //
-  // void main()
-  // {
-  //     while (true) {
-  //         break;
-  //     }
-  //     gl_FragColor = BaseColor;
-  // }
-
-  const std::string assembly =
-      R"(OpCapability Shader
-%1 = OpExtInstImport "GLSL.std.450"
-OpMemoryModel Logical GLSL450
-OpEntryPoint Fragment %main "main" %gl_FragColor %BaseColor
-OpExecutionMode %main OriginUpperLeft
-OpSource GLSL 140
-OpName %main "main"
-OpName %gl_FragColor "gl_FragColor"
-OpName %BaseColor "BaseColor"
-%void = OpTypeVoid
-%6 = OpTypeFunction %void
-%bool = OpTypeBool
-%true = OpConstantTrue %bool
-%float = OpTypeFloat 32
-%v4float = OpTypeVector %float 4
-%_ptr_Output_v4float = OpTypePointer Output %v4float
-%gl_FragColor = OpVariable %_ptr_Output_v4float Output
-%_ptr_Input_v4float = OpTypePointer Input %v4float
-%BaseColor = OpVariable %_ptr_Input_v4float Input
-%main = OpFunction %void None %6
-%13 = OpLabel
-OpBranch %14
-%14 = OpLabel
-OpLoopMerge %15 %16 None
-OpBranch %17
-%17 = OpLabel
-OpBranch %15
-%18 = OpLabel
-OpBranch %16
-%16 = OpLabel
-OpBranch %14
-%15 = OpLabel
-%19 = OpLoad %v4float %BaseColor
-OpStore %gl_FragColor %19
-OpReturn
-OpFunctionEnd
-)";
-
-  SinglePassRunAndCheck<opt::BlockMergePass>(assembly, assembly, true, true);
-}
-
 TEST_F(BlockMergeTest, NestedInControlFlow) {
   // Note: SPIR-V hand edited to insert block boundary
   // between OpFMul and OpStore in then-part.
@@ -328,6 +271,255 @@ OpFunctionEnd
                                              true, true);
 }
 
+#ifdef SPIRV_EFFCEE
+TEST_F(BlockMergeTest, PhiInSuccessorOfMergedBlock) {
+  const std::string text = R"(
+; CHECK: OpSelectionMerge [[merge:%\w+]] None
+; CHECK-NEXT: OpBranchConditional {{%\w+}} [[then:%\w+]] [[else:%\w+]]
+; CHECK: [[then]] = OpLabel
+; CHECK-NEXT: OpBranch [[merge]]
+; CHECK: [[else]] = OpLabel
+; CHECK-NEXT: OpBranch [[merge]]
+; CHECK: [[merge]] = OpLabel
+; CHECK-NEXT: OpPhi {{%\w+}} %true [[then]] %false [[else]]
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse  %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%entry = OpLabel
+OpSelectionMerge %merge None
+OpBranchConditional %true %then %else
+%then = OpLabel
+OpBranch %then_next
+%then_next = OpLabel
+OpBranch %merge
+%else = OpLabel
+OpBranch %merge
+%merge = OpLabel
+%phi = OpPhi %bool %true %then_next %false %else
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<opt::BlockMergePass>(text, true);
+}
+
+TEST_F(BlockMergeTest, UpdateMergeInstruction) {
+  const std::string text = R"(
+; CHECK: OpSelectionMerge [[merge:%\w+]] None
+; CHECK-NEXT: OpBranchConditional {{%\w+}} [[then:%\w+]] [[else:%\w+]]
+; CHECK: [[then]] = OpLabel
+; CHECK-NEXT: OpBranch [[merge]]
+; CHECK: [[else]] = OpLabel
+; CHECK-NEXT: OpBranch [[merge]]
+; CHECK: [[merge]] = OpLabel
+; CHECK-NEXT: OpReturn
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse  %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%entry = OpLabel
+OpSelectionMerge %real_merge None
+OpBranchConditional %true %then %else
+%then = OpLabel
+OpBranch %merge
+%else = OpLabel
+OpBranch %merge
+%merge = OpLabel
+OpBranch %real_merge
+%real_merge = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<opt::BlockMergePass>(text, true);
+}
+
+TEST_F(BlockMergeTest, TwoMergeBlocksCannotBeMerged) {
+  const std::string text = R"(
+; CHECK: OpSelectionMerge [[outer_merge:%\w+]] None
+; CHECK: OpSelectionMerge [[inner_merge:%\w+]] None
+; CHECK: [[inner_merge]] = OpLabel
+; CHECK-NEXT: OpBranch [[outer_merge]]
+; CHECK: [[outer_merge]] = OpLabel
+; CHECK-NEXT: OpReturn
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse  %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%entry = OpLabel
+OpSelectionMerge %outer_merge None
+OpBranchConditional %true %then %else
+%then = OpLabel
+OpBranch %inner_header
+%else = OpLabel
+OpBranch %inner_header
+%inner_header = OpLabel
+OpSelectionMerge %inner_merge None
+OpBranchConditional %true %inner_then %inner_else
+%inner_then = OpLabel
+OpBranch %inner_merge
+%inner_else = OpLabel
+OpBranch %inner_merge
+%inner_merge = OpLabel
+OpBranch %outer_merge
+%outer_merge = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<opt::BlockMergePass>(text, true);
+}
+
+TEST_F(BlockMergeTest, MergeContinue) {
+  const std::string text = R"(
+; CHECK: OpBranch [[header:%\w+]]
+; CHECK: [[header]] = OpLabel
+; CHECK-NEXT: OpLogicalAnd
+; CHECK-NEXT: OpLoopMerge {{%\w+}} [[header]] None
+; CHECK-NEXT: OpBranch [[header]]
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse  %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%entry = OpLabel
+OpBranch %header
+%header = OpLabel
+OpLoopMerge %merge %continue None
+OpBranch %continue
+%continue = OpLabel
+%op = OpLogicalAnd %bool %true %false
+OpBranch %header
+%merge = OpLabel
+OpUnreachable
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<opt::BlockMergePass>(text, true);
+}
+
+TEST_F(BlockMergeTest, TwoHeadersCannotBeMerged) {
+  const std::string text = R"(
+; CHECK: OpBranch [[loop_header:%\w+]]
+; CHECK: [[loop_header]] = OpLabel
+; CHECK-NEXT: OpLoopMerge
+; CHECK-NEXT: OpBranch [[if_header:%\w+]]
+; CHECK: [[if_header]] = OpLabel
+; CHECK-NEXT: OpSelectionMerge
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse  %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%entry = OpLabel
+OpBranch %header
+%header = OpLabel
+OpLoopMerge %merge %continue None
+OpBranch %inner_header
+%inner_header = OpLabel
+OpSelectionMerge %continue None
+OpBranchConditional %true %then %continue
+%then = OpLabel
+OpBranch %continue
+%continue = OpLabel
+OpBranchConditional %false %merge %header
+%merge = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<opt::BlockMergePass>(text, true);
+}
+
+TEST_F(BlockMergeTest, RemoveStructuredDeclaration) {
+  // Note: SPIR-V hand edited remove dead branch and add block
+  // before continue block
+  //
+  // #version 140
+  // in vec4 BaseColor;
+  //
+  // void main()
+  // {
+  //     while (true) {
+  //         break;
+  //     }
+  //     gl_FragColor = BaseColor;
+  // }
+
+  const std::string assembly =
+      R"(
+; CHECK: OpLabel
+; CHECK: [[header:%\w+]] = OpLabel
+; CHECK-NOT: OpLoopMerge
+; CHECK: OpReturn
+; CHECK: [[continue:%\w+]] = OpLabel
+; CHECK-NEXT: OpBranch [[header]]
+OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main" %gl_FragColor %BaseColor
+OpExecutionMode %main OriginUpperLeft
+OpSource GLSL 140
+OpName %main "main"
+OpName %gl_FragColor "gl_FragColor"
+OpName %BaseColor "BaseColor"
+%void = OpTypeVoid
+%6 = OpTypeFunction %void
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%float = OpTypeFloat 32
+%v4float = OpTypeVector %float 4
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+%gl_FragColor = OpVariable %_ptr_Output_v4float Output
+%_ptr_Input_v4float = OpTypePointer Input %v4float
+%BaseColor = OpVariable %_ptr_Input_v4float Input
+%main = OpFunction %void None %6
+%13 = OpLabel
+OpBranch %14
+%14 = OpLabel
+OpLoopMerge %15 %16 None
+OpBranch %17
+%17 = OpLabel
+OpBranch %15
+%18 = OpLabel
+OpBranch %16
+%16 = OpLabel
+OpBranch %14
+%15 = OpLabel
+%19 = OpLoad %v4float %BaseColor
+OpStore %gl_FragColor %19
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<opt::BlockMergePass>(assembly, true);
+}
+#endif  // SPIRV_EFFCEE
+
 // TODO(greg-lunarg): Add tests to verify handling of these cases:
 //
 //    More complex control flow