Fix block ordering in dead branch elim

Fixes #1727

* If the pass finds any dead branches it can optimize then at the end of
the pass it reorders basic blocks to ensure they satisfy block ordering
requirements
 * Added some new tests
* While investigating this issue, found and fixed a non-deterministic
ordering of dominators
 * Now the edges used to construct the dominator tree are sorted
 according to posorder traversal indices

source/cfa.h

@@ -244,6 +244,22 @@ vector<pair<BB*, BB*>> CFA<BB>::CalculateDominators(
     out.push_back({const_cast<BB*>(get<0>(idom)),
                    const_cast<BB*>(postorder[get<1>(idom).dominator])});
   }
+
+  // Sort by postorder index to generate a deterministic ordering of edges.
+  std::sort(
+      out.begin(), out.end(),
+      [&idoms](const pair<bb_ptr, bb_ptr>& lhs,
+               const pair<bb_ptr, bb_ptr>& rhs) {
+        assert(lhs.first);
+        assert(lhs.second);
+        assert(rhs.first);
+        assert(rhs.second);
+        auto lhs_indices = std::make_pair(idoms[lhs.first].postorder_index,
+                                          idoms[lhs.second].postorder_index);
+        auto rhs_indices = std::make_pair(idoms[rhs.first].postorder_index,
+                                          idoms[rhs.second].postorder_index);
+        return lhs_indices < rhs_indices;
+      });
   return out;
 }
 

source/opt/dead_branch_elim_pass.cpp

@@ -364,6 +364,48 @@ bool DeadBranchElimPass::EliminateDeadBranches(Function* func) {
   return modified;
 }
 
+void DeadBranchElimPass::FixBlockOrder() {
+  context()->BuildInvalidAnalyses(IRContext::kAnalysisCFG |
+                                  IRContext::kAnalysisDominatorAnalysis);
+  // Reorders blocks according to DFS of dominator tree.
+  ProcessFunction reorder_dominators = [this](Function* function) {
+    DominatorAnalysis* dominators = context()->GetDominatorAnalysis(function);
+    std::vector<BasicBlock*> blocks;
+    for (auto iter = dominators->GetDomTree().begin();
+         iter != dominators->GetDomTree().end(); ++iter) {
+      if (iter->id() != 0) {
+        blocks.push_back(iter->bb_);
+      }
+    }
+    for (uint32_t i = 1; i < blocks.size(); ++i) {
+      function->MoveBasicBlockToAfter(blocks[i]->id(), blocks[i - 1]);
+    }
+    return true;
+  };
+
+  // Reorders blocks according to structured order.
+  ProcessFunction reorder_structured = [this](Function* function) {
+    std::list<BasicBlock*> order;
+    context()->cfg()->ComputeStructuredOrder(function, &*function->begin(),
+                                             &order);
+    std::vector<BasicBlock*> blocks;
+    for (auto block : order) {
+      blocks.push_back(block);
+    }
+    for (uint32_t i = 1; i < blocks.size(); ++i) {
+      function->MoveBasicBlockToAfter(blocks[i]->id(), blocks[i - 1]);
+    }
+    return true;
+  };
+
+  // Structured order is more intuitive so use it where possible.
+  if (context()->get_feature_mgr()->HasCapability(SpvCapabilityShader)) {
+    ProcessReachableCallTree(reorder_structured, context());
+  } else {
+    ProcessReachableCallTree(reorder_dominators, context());
+  }
+}
+
 Pass::Status DeadBranchElimPass::Process() {
   // Do not process if module contains OpGroupDecorate. Additional
   // support required in KillNamesAndDecorates().
@@ -375,6 +417,7 @@ Pass::Status DeadBranchElimPass::Process() {
     return EliminateDeadBranches(fp);
   };
   bool modified = ProcessReachableCallTree(pfn, context());
+  if (modified) FixBlockOrder();
   return modified ? Status::SuccessWithChange : Status::SuccessWithoutChange;
 }
 

source/opt/dead_branch_elim_pass.h

@@ -125,6 +125,10 @@ class DeadBranchElimPass : public MemPass {
       const std::unordered_set<BasicBlock*>& unreachable_merges,
       const std::unordered_map<BasicBlock*, BasicBlock*>&
           unreachable_continues);
+
+  // Reorders blocks in reachable functions so that they satisfy dominator
+  // block ordering rules.
+  void FixBlockOrder();
 };
 
 }  // namespace opt

test/opt/dead_branch_elim_test.cpp

@@ -605,12 +605,12 @@ OpBranch %28
 %37 = OpINotEqual %bool %36 %uint_0
 OpSelectionMerge %38 None
 OpBranchConditional %37 %39 %40
-%39 = OpLabel
-OpStore %v %23
-OpBranch %38
 %40 = OpLabel
 OpStore %v %21
 OpBranch %38
+%39 = OpLabel
+OpStore %v %23
+OpBranch %38
 %38 = OpLabel
 OpBranch %26
 %26 = OpLabel
@@ -1654,12 +1654,12 @@ TEST_F(DeadBranchElimTest, RemoveUnreachableBlocksPartiallyDeadPhi) {
 ; CHECK-NEXT: [[param:%\w+]] = OpFunctionParameter
 ; CHECK-NEXT: OpLabel
 ; CHECK-NEXT: OpBranchConditional [[param]] [[merge:%\w+]] [[br:%\w+]]
-; CHECK-NEXT: [[br]] = OpLabel
-; CHECK-NEXT: OpBranch [[merge]]
 ; CHECK-NEXT: [[merge]] = OpLabel
 ; CHECK-NEXT: [[phi:%\w+]] = OpPhi %bool %true %2 %false [[br]]
 ; CHECK-NEXT: OpLogicalNot %bool [[phi]]
 ; CHECK-NEXT: OpReturn
+; CHECK-NEXT: [[br]] = OpLabel
+; CHECK-NEXT: OpBranch [[merge]]
 ; CHECK-NEXT: OpFunctionEnd
 OpCapability Kernel
 OpCapability Linkage
@@ -1772,9 +1772,10 @@ TEST_F(DeadBranchElimTest, ExtraBackedgeBlocksUnreachable) {
 ; CHECK-NEXT: [[header]] = OpLabel
 ; CHECK-NEXT: OpLoopMerge [[merge:%\w+]] [[continue:%\w+]] None
 ; CHECK-NEXT: OpBranch [[merge]]
+; CHECK-NEXT: [[merge]] = OpLabel
+; CHECK-NEXT: OpReturn
 ; CHECK-NEXT: [[continue]] = OpLabel
 ; CHECK-NEXT: OpBranch [[header]]
-; CHECK-NEXT: [[merge]] = OpLabel
 OpCapability Kernel
 OpCapability Linkage
 OpMemoryModel Logical OpenCL
@@ -1841,6 +1842,7 @@ TEST_F(DeadBranchElimTest, ExtraBackedgePartiallyDead) {
 ; CHECK: OpLabel
 ; CHECK: [[header:%\w+]] = OpLabel
 ; CHECK: OpLoopMerge [[merge:%\w+]] [[continue:%\w+]] None
+; CHECK: [[merge]] = OpLabel
 ; CHECK: [[continue]] = OpLabel
 ; CHECK: OpBranch [[extra:%\w+]]
 ; CHECK: [[extra]] = OpLabel
@@ -1851,7 +1853,6 @@ TEST_F(DeadBranchElimTest, ExtraBackedgePartiallyDead) {
 ; CHECK-NEXT: OpBranch [[backedge:%\w+]]
 ; CHECK-NEXT: [[backedge:%\w+]] = OpLabel
 ; CHECK-NEXT: OpBranch [[header]]
-; CHECK-NEXT: [[merge]] = OpLabel
 OpCapability Kernel
 OpCapability Linkage
 OpMemoryModel Logical OpenCL
@@ -2002,6 +2003,95 @@ OpFunctionEnd
 
   SinglePassRunAndMatch<DeadBranchElimPass>(text, true);
 }
+
+TEST_F(DeadBranchElimTest, ReorderBlocks) {
+  const std::string text = R"(
+; CHECK: OpLabel
+; CHECK: OpBranch [[label:%\w+]]
+; CHECK: [[label:%\w+]] = OpLabel
+; CHECK-NEXT: OpLogicalNot
+; CHECK-NEXT: OpBranch [[label:%\w+]]
+; CHECK: [[label]] = OpLabel
+; CHECK-NEXT: OpReturn
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%func_ty = OpTypeFunction %void
+%func = OpFunction %void None %func_ty
+%1 = OpLabel
+OpSelectionMerge %3 None
+OpBranchConditional %true %2 %3
+%3 = OpLabel
+OpReturn
+%2 = OpLabel
+%not = OpLogicalNot %bool %true
+OpBranch %3
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<DeadBranchElimPass>(text, true);
+}
+
+TEST_F(DeadBranchElimTest, ReorderBlocksMultiple) {
+  // Checks are not important. The validation post optimization is the
+  // important part.
+  const std::string text = R"(
+; CHECK: OpLabel
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%func_ty = OpTypeFunction %void
+%func = OpFunction %void None %func_ty
+%1 = OpLabel
+OpSelectionMerge %3 None
+OpBranchConditional %true %2 %3
+%3 = OpLabel
+OpReturn
+%2 = OpLabel
+OpBranch %4
+%4 = OpLabel
+OpBranch %3
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<DeadBranchElimPass>(text, true);
+}
+
+TEST_F(DeadBranchElimTest, ReorderBlocksMultiple2) {
+  // Checks are not important. The validation post optimization is the
+  // important part.
+  const std::string text = R"(
+; CHECK: OpLabel
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func"
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%func_ty = OpTypeFunction %void
+%func = OpFunction %void None %func_ty
+%1 = OpLabel
+OpSelectionMerge %3 None
+OpBranchConditional %true %2 %3
+%3 = OpLabel
+OpBranch %5
+%5 = OpLabel
+OpReturn
+%2 = OpLabel
+OpBranch %4
+%4 = OpLabel
+OpBranch %3
+OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<DeadBranchElimPass>(text, true);
+}
 #endif
 
 // TODO(greg-lunarg): Add tests to verify handling of these cases: