Merge pull request #122 from HansKristian-Work/cfg-fixes

Handle more scenarios where natural merge != merge in switch blocks.

cfg_structurizer.cpp

@@ -2572,7 +2572,7 @@ void CFGStructurizer::recompute_cfg()
 	compute_post_dominance_frontier();
 }
 
-CFGNode *CFGStructurizer::find_natural_switch_merge_block(CFGNode *node, CFGNode *post_dominator)
+CFGNode *CFGStructurizer::find_natural_switch_merge_block(CFGNode *node, CFGNode *post_dominator) const
 {
 	// Maintain the original switch block order if possible to avoid awkward churn in reference output.
 	uint64_t order = 1;
@@ -2599,7 +2599,8 @@ CFGNode *CFGStructurizer::find_natural_switch_merge_block(CFGNode *node, CFGNode
 
 			// A case label might be the merge block candidate of the switch.
 			// Don't consider case fallthrough if b post-dominates the entire switch statement.
-			if (child.node != post_dominator && parent.node != child.node && child.node->can_backtrace_to(parent.node))
+			if (child.node != post_dominator && parent.node != child.node &&
+			    query_reachability(*parent.node, *child.node))
 			{
 				parent.global_order = child.global_order - 1;
 				break;
@@ -2635,6 +2636,38 @@ CFGNode *CFGStructurizer::find_natural_switch_merge_block(CFGNode *node, CFGNode
 			if (c.global_order == target_order)
 				return c.node;
 
+	// If two case labels merge execution before the candidate merge, we should consider that the natural merge,
+	// since it is not possible to express this without a switch merge.
+	for (auto &c : node->ir.terminator.cases)
+	{
+		for (auto *front : c.node->dominance_frontier)
+		{
+			// Ignore frontiers that are other case labels.
+			// We allow simple fallthrough, and if we found an impossible case we would have handled it already.
+			for (auto &ic : node->ir.terminator.cases)
+			{
+				if (ic.node == front)
+				{
+					front = nullptr;
+					break;
+				}
+			}
+
+			if (!front)
+				continue;
+
+			if (front->forward_post_visit_order != post_dominator->forward_post_visit_order &&
+			    query_reachability(*front, *post_dominator))
+			{
+				// If this is reachable by a different case label, we have a winner. This must be a fake fallthrough
+				// that we should promote to switch merge.
+				for (auto &ic : node->ir.terminator.cases)
+					if (ic.node != c.node && query_reachability(*ic.node, *front))
+						return front;
+			}
+		}
+	}
+
 	return post_dominator;
 }
 
@@ -2665,27 +2698,36 @@ bool CFGStructurizer::find_switch_blocks(unsigned pass)
 				switch_outer->freeze_structured_analysis = true;
 				merge->headers.push_back(switch_outer);
 
-				auto *dummy_case = pool.create_node();
-				dummy_case->name = natural_merge->name + ".pred";
-				dummy_case->immediate_dominator = node;
-				dummy_case->immediate_post_dominator = natural_merge;
-				dummy_case->forward_post_visit_order = node->forward_post_visit_order;
-				dummy_case->backward_post_visit_order = node->backward_post_visit_order;
-				dummy_case->ir.terminator.type = Terminator::Type::Branch;
-				dummy_case->ir.terminator.direct_block = natural_merge;
-				dummy_case->add_branch(natural_merge);
-				node->retarget_branch(natural_merge, dummy_case);
-
-				auto *dummy_break = pool.create_node();
-				dummy_break->name = node->name + ".break";
-				dummy_break->immediate_dominator = node;
-				dummy_break->immediate_post_dominator = merge;
-				dummy_break->forward_post_visit_order = node->forward_post_visit_order;
-				dummy_break->backward_post_visit_order = node->backward_post_visit_order;
-				dummy_break->ir.terminator.type = Terminator::Type::Branch;
-				dummy_break->ir.terminator.direct_block = merge;
-				dummy_break->add_branch(merge);
-				node->retarget_branch(merge, dummy_break);
+				// Shouldn't be needed (I believe), but spirv-val is a bit temperamental when double breaking
+				// straight out of a switch block in some situations,
+				// so try not to ruffle too many feathers.
+				if (std::find(node->succ.begin(), node->succ.end(), natural_merge) != node->succ.end())
+				{
+					auto *dummy_case = pool.create_node();
+					dummy_case->name = natural_merge->name + ".pred";
+					dummy_case->immediate_dominator = node;
+					dummy_case->immediate_post_dominator = natural_merge;
+					dummy_case->forward_post_visit_order = node->forward_post_visit_order;
+					dummy_case->backward_post_visit_order = node->backward_post_visit_order;
+					dummy_case->ir.terminator.type = Terminator::Type::Branch;
+					dummy_case->ir.terminator.direct_block = natural_merge;
+					dummy_case->add_branch(natural_merge);
+					node->retarget_branch(natural_merge, dummy_case);
+				}
+
+				if (std::find(node->succ.begin(), node->succ.end(), merge) != node->succ.end())
+				{
+					auto *dummy_break = pool.create_node();
+					dummy_break->name = node->name + ".break";
+					dummy_break->immediate_dominator = node;
+					dummy_break->immediate_post_dominator = merge;
+					dummy_break->forward_post_visit_order = node->forward_post_visit_order;
+					dummy_break->backward_post_visit_order = node->backward_post_visit_order;
+					dummy_break->ir.terminator.type = Terminator::Type::Branch;
+					dummy_break->ir.terminator.direct_block = merge;
+					dummy_break->add_branch(merge);
+					node->retarget_branch(merge, dummy_break);
+				}
 
 				node->freeze_structured_analysis = true;
 			}

cfg_structurizer.hpp

@@ -115,7 +115,7 @@ class CFGStructurizer
 	bool header_and_merge_block_have_entry_exit_relationship(const CFGNode *header, const CFGNode *merge) const;
 	void fixup_broken_selection_merges(unsigned pass);
 	bool find_switch_blocks(unsigned pass);
-	static CFGNode *find_natural_switch_merge_block(CFGNode *node, CFGNode *post_dominator);
+	CFGNode *find_natural_switch_merge_block(CFGNode *node, CFGNode *post_dominator) const;
 	const CFGNode *get_innermost_loop_header_for(const CFGNode *node) const;
 	const CFGNode *get_innermost_loop_header_for(const CFGNode *header, const CFGNode *node) const;
 	bool loop_exit_supports_infinite_loop(const CFGNode *header, const CFGNode *loop_exit) const;