Merge pull request #97 from HansKristian-Work/interlocked-merge-scope…

…-fix

Interlocked merge scope fix

bc/instruction.cpp

@@ -420,11 +420,12 @@ AtomicRMWInst::BinOp AtomicRMWInst::getOperation() const
 }
 
 AtomicCmpXchgInst::AtomicCmpXchgInst(Value *ptr_, Value *cmp_, Value *new_value_)
-    : Instruction(StructType::get({ new_value_->getType(), Type::getInt1Ty(new_value_->getType()->getContext()) }),
-                  ValueKind::AtomicCmpXchg)
-    , ptr(ptr_)
-    , new_value(new_value_)
-    , cmp_value(cmp_)
+	: Instruction(StructType::get(new_value_->getType()->getContext(),
+								  { new_value_->getType(), Type::getInt1Ty(new_value_->getType()->getContext()) }),
+				  ValueKind::AtomicCmpXchg)
+	, ptr(ptr_)
+	, new_value(new_value_)
+	, cmp_value(cmp_)
 {
 }
 

bc/module.cpp

@@ -1960,7 +1960,7 @@ bool ModuleParseContext::parse_type(const BlockOrRecord &child)
 		members.reserve(num_members);
 		for (unsigned i = 0; i < num_members; i++)
 			members.push_back(get_type(child.ops[i + 1]));
-		type = StructType::get(std::move(members));
+		type = StructType::get(*context, std::move(members));
 		break;
 	}
 

bc/type.cpp

@@ -180,10 +180,8 @@ Type *StructType::getElementType(unsigned N) const
 	return member_types[N];
 }
 
-StructType *StructType::get(Vector<Type *> member_types)
+StructType *StructType::get(LLVMContext &context, Vector<Type *> member_types)
 {
-	assert(!member_types.empty());
-	auto &context = member_types.front()->getContext();
 	auto &cache = context.get_type_cache();
 	for (auto *type : cache)
 	{

bc/type.hpp

@@ -135,7 +135,7 @@ class StructType : public Type
 		return TypeID::StructTyID;
 	}
 	StructType(LLVMContext &context, Vector<Type *> member_types);
-	static StructType *get(Vector<Type *> member_types);
+	static StructType *get(LLVMContext &context, Vector<Type *> member_types);
 
 	unsigned getNumElements() const;
 	Type *getElementType(unsigned N) const;

cfg_structurizer.cpp

@@ -433,6 +433,14 @@ CFGNode *CFGStructurizer::get_entry_block() const
 	return entry_block;
 }
 
+static bool block_is_control_dependent(const CFGNode *node)
+{
+	for (auto *op : node->ir.operations)
+		if (SPIRVModule::opcode_is_control_dependent(op->op))
+			return true;
+	return false;
+}
+
 bool CFGStructurizer::continue_block_can_merge(CFGNode *node) const
 {
 	const CFGNode *pred_candidate = nullptr;
@@ -642,8 +650,12 @@ void CFGStructurizer::duplicate_impossible_merge_constructs()
 		// If the candidate has control dependent effects like barriers and such,
 		// this will likely break completely,
 		// but I don't see how that would work on native drivers either ...
-		if (merge_candidate_is_on_loop_breaking_path(node) && !node->ir.operations.empty())
+		if (merge_candidate_is_on_loop_breaking_path(node) &&
+		    !node->ir.operations.empty() &&
+		    !block_is_control_dependent(node))
+		{
 			duplicate_queue.push_back(node);
+		}
 	}
 
 	if (duplicate_queue.empty())
@@ -1695,7 +1707,7 @@ bool CFGStructurizer::control_flow_is_escaping(const CFGNode *node, const CFGNod
 	return escaping_path;
 }
 
-bool CFGStructurizer::block_is_plain_continue(const CFGNode *node) const
+bool CFGStructurizer::block_is_plain_continue(const CFGNode *node)
 {
 	return node->succ_back_edge != nullptr && node != node->succ_back_edge;
 }
@@ -1771,8 +1783,8 @@ void CFGStructurizer::fixup_broken_selection_merges(unsigned pass)
 			if (merge)
 			{
 				bool dominates_merge = node->dominates(merge);
-				bool merges_to_continue = merge && merge->succ_back_edge;
-				if (dominates_merge && !merge->headers.empty())
+				bool merges_to_continue = block_is_plain_continue(merge);
+				if (!merges_to_continue && dominates_merge && !merge->headers.empty())
 				{
 					// Here we have a likely case where one block is doing a clean "break" out of a loop, and
 					// the other path continues as normal, and then conditionally breaks in a continue block or something similar.
@@ -1883,7 +1895,7 @@ void CFGStructurizer::fixup_broken_selection_merges(unsigned pass)
 		if (trivial_merge_index >= 0 && pass == 0)
 		{
 			CFGNode *merge = CFGStructurizer::find_common_post_dominator(node->succ);
-			if (merge && !node->dominates(merge))
+			if (merge && !node->dominates(merge) && !block_is_plain_continue(merge))
 			{
 				if (!merge->headers.empty())
 				{
@@ -2025,35 +2037,132 @@ void CFGStructurizer::rewrite_selection_breaks(CFGNode *header, CFGNode *ladder_
 	}
 }
 
-bool CFGStructurizer::header_and_merge_block_have_entry_exit_relationship(const CFGNode *header, const CFGNode *merge)
+bool CFGStructurizer::header_and_merge_block_have_entry_exit_relationship(const CFGNode *header, const CFGNode *merge) const
 {
 	if (!merge->post_dominates(header))
 		return false;
 
 	// If there are other blocks which need merging, and that idom is the header,
 	// then header is some kind of exit block.
 	bool found_inner_merge_target = false;
+	const CFGNode *potential_inner_merge_target = nullptr;
 
 	UnorderedSet<const CFGNode *> traversed;
 
+	const auto is_earlier = [](const CFGNode *candidate, const CFGNode *existing) {
+		return !existing || (candidate->forward_post_visit_order > existing->forward_post_visit_order);
+	};
+
+	const auto is_later = [](const CFGNode *candidate, const CFGNode *existing) {
+		return !existing || (candidate->forward_post_visit_order < existing->forward_post_visit_order);
+	};
+
 	header->traverse_dominated_blocks([&](const CFGNode *node) {
 		if (node == merge)
 			return false;
 		if (traversed.count(node))
 			return false;
 		traversed.insert(node);
 
+		// Don't analyze loops, this path is mostly for selections only.
+		if (node->pred_back_edge)
+			return false;
+
 		if (node->num_forward_preds() <= 1)
 			return true;
 		auto *idom = node->immediate_dominator;
+
 		if (idom == header)
 		{
 			found_inner_merge_target = true;
 			return false;
 		}
+		else if (is_later(node, potential_inner_merge_target) &&
+		         idom->immediate_post_dominator == merge &&
+		         !exists_path_in_cfg_without_intermediate_node(header, node, idom))
+		{
+			// Need to analyze this further to determine if it's one of those insane crossing merge cases ...
+			// Find the lowest post visit order if there are multiple candidates.
+			potential_inner_merge_target = node;
+		}
+
 		return true;
 	});
-	return found_inner_merge_target;
+
+	if (found_inner_merge_target)
+		return true;
+	if (!potential_inner_merge_target)
+		return false;
+
+	// Alternatively, try to find a situation where the natural merge is difficult to determine.
+	// In this scenario, selection constructs appear to be "breaking" in different directions.
+	// Any attempt to split scopes here will fail spectacularly.
+
+	const CFGNode *first_natural_breaks_to_outer = nullptr;
+	const CFGNode *first_natural_breaks_to_inner = nullptr;
+	const CFGNode *last_natural_breaks_to_outer = nullptr;
+	const CFGNode *last_natural_breaks_to_inner = nullptr;
+	traversed.clear();
+
+	header->traverse_dominated_blocks([&](const CFGNode *node) {
+		if (node == merge || node == potential_inner_merge_target)
+			return false;
+		if (!query_reachability(*node, *merge) || !query_reachability(*node, *potential_inner_merge_target))
+			return false;
+		if (traversed.count(node))
+			return false;
+		traversed.insert(node);
+
+		if (node->succ.size() < 2)
+			return true;
+
+		bool breaks_to_outer = std::find_if(node->succ.begin(), node->succ.end(), [&](const CFGNode *candidate) {
+			return merge->post_dominates(candidate);
+		}) != node->succ.end();
+
+		bool breaks_to_inner = std::find_if(node->succ.begin(), node->succ.end(), [&](const CFGNode *candidate) {
+			return potential_inner_merge_target->post_dominates(candidate);
+		}) != node->succ.end();
+
+		if (breaks_to_inner)
+			breaks_to_outer = false;
+
+		if (breaks_to_outer)
+		{
+			if (is_earlier(node, first_natural_breaks_to_outer))
+				first_natural_breaks_to_outer = node;
+			if (is_later(node, last_natural_breaks_to_outer))
+				last_natural_breaks_to_outer = node;
+		}
+
+		if (breaks_to_inner)
+		{
+			if (is_earlier(node, first_natural_breaks_to_inner))
+				first_natural_breaks_to_inner = node;
+			if (is_later(node, last_natural_breaks_to_inner))
+				last_natural_breaks_to_inner = node;
+		}
+
+		return true;
+	});
+
+	if (!first_natural_breaks_to_outer || !first_natural_breaks_to_inner ||
+	    !last_natural_breaks_to_outer || !last_natural_breaks_to_inner)
+	{
+		return false;
+	}
+
+	const auto is_ordered = [](const CFGNode *a, const CFGNode *b, const CFGNode *c) {
+		return a != b && a->dominates(b) && b != c && b->dominates(c);
+	};
+
+	// Crossing break scenario.
+	if (is_ordered(first_natural_breaks_to_inner, first_natural_breaks_to_outer, last_natural_breaks_to_inner))
+		return true;
+	else if (is_ordered(first_natural_breaks_to_outer, first_natural_breaks_to_inner, last_natural_breaks_to_outer))
+		return true;
+	else
+		return false;
 }
 
 void CFGStructurizer::split_merge_scopes()
@@ -2065,6 +2174,9 @@ void CFGStructurizer::split_merge_scopes()
 		if (node->num_forward_preds() <= 1)
 			continue;
 
+		if (block_is_plain_continue(node))
+			continue;
+
 		// The idom is the natural header block.
 		auto *idom = node->immediate_dominator;
 		assert(idom->succ.size() >= 2);
@@ -2286,7 +2398,7 @@ bool CFGStructurizer::find_switch_blocks(unsigned pass)
 		merge = natural_merge;
 
 		// We cannot rewrite the CFG in pass 1 safely, this should have happened in pass 0.
-		if (pass == 0 && !node->dominates(merge))
+		if (pass == 0 && (!node->dominates(merge) || block_is_plain_continue(merge)))
 		{
 			// We did not rewrite switch blocks w.r.t. selection breaks.
 			// We might be in a situation where the switch block is trying to merge to a block which is already being merged to.
@@ -2583,7 +2695,12 @@ CFGNode *CFGStructurizer::create_helper_pred_block(CFGNode *node)
 		header->fixup_merge_info_after_branch_rewrite(node, pred_node);
 	node->headers.clear();
 
-	pred_node->immediate_dominator = node->immediate_dominator;
+	// We're replacing entry block.
+	if (node == node->immediate_dominator)
+		pred_node->immediate_dominator = pred_node;
+	else
+		pred_node->immediate_dominator = node->immediate_dominator;
+
 	pred_node->immediate_post_dominator = node;
 	node->immediate_dominator = pred_node;
 
@@ -3242,6 +3359,8 @@ void CFGStructurizer::split_merge_blocks()
 		if (node->headers.size() <= 1)
 			continue;
 
+		assert(!block_is_plain_continue(node));
+
 		// If this block was the merge target for more than one construct,
 		// we will need to split the block. In SPIR-V, a merge block can only be the merge target for one construct.
 		// However, we can set up a chain of merges where inner scope breaks to outer scope with a dummy basic block.

cfg_structurizer.hpp

@@ -79,7 +79,7 @@ class CFGStructurizer
 	Operation *duplicate_op(Operation *op, UnorderedMap<spv::Id, spv::Id> &id_remap);
 	void update_structured_loop_merge_targets();
 	void find_selection_merges(unsigned pass);
-	static bool header_and_merge_block_have_entry_exit_relationship(const CFGNode *header, const CFGNode *merge);
+	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);
@@ -88,7 +88,7 @@ class CFGStructurizer
 	bool merge_candidate_is_on_breaking_path(const CFGNode *node) const;
 	bool merge_candidate_is_on_loop_breaking_path(const CFGNode *node) const;
 	bool continue_block_can_merge(CFGNode *node) const;
-	bool block_is_plain_continue(const CFGNode *node) const;
+	static bool block_is_plain_continue(const CFGNode *node);
 	CFGNode *get_target_break_block_for_inner_header(const CFGNode *node, size_t header_index);
 	CFGNode *get_or_create_ladder_block(CFGNode *node, size_t header_index);
 	CFGNode *build_enclosing_break_target_for_loop_ladder(CFGNode *&node, CFGNode *loop_ladder);

dxil_converter.cpp

@@ -2335,7 +2335,14 @@ void Converter::Impl::get_shader_model(const llvm::Module &module, String *model
 		auto *meta = resource_meta->getOperand(0);
 
 		if (model)
-			*model = llvm::cast<llvm::MDString>(meta->getOperand(0))->getString();
+		{
+			auto str = llvm::cast<llvm::MDString>(meta->getOperand(0))->getString();
+#ifdef HAVE_LLVMBC
+			*model = std::move(str);
+#else
+			*model = String(str.begin(), str.end());
+#endif
+		}
 		if (major)
 			*major = get_constant_metadata(meta, 1);
 		if (minor)