shader: canonicalize CFG loop headers

src/graphics/shader/recompiler/ShaderRecompiler.cpp

@@ -78,85 +78,6 @@ void LogDispatcherFallback(const CompileOptions& options, const CFG::Graph& cfg,
 	     static_cast<uint64_t>(cfg.back_edges.size()), reason.c_str());
 }
 
-bool InstructionMaySplitSpirvBlock(const IR::Instruction& inst) {
-	switch (inst.op) {
-		case IR::Opcode::SLoadDword:
-		case IR::Opcode::SBufferLoadDword:
-		case IR::Opcode::BufferLoadUbyte:
-		case IR::Opcode::BufferLoadSbyte:
-		case IR::Opcode::BufferLoadUshort:
-		case IR::Opcode::BufferLoadSshort:
-		case IR::Opcode::BufferLoadDword:
-		case IR::Opcode::BufferStoreByte:
-		case IR::Opcode::BufferStoreShort:
-		case IR::Opcode::BufferStoreDword:
-		case IR::Opcode::AtomicSwapU32:
-		case IR::Opcode::AtomicAddU32:
-		case IR::Opcode::AtomicSubU32:
-		case IR::Opcode::AtomicSMinI32:
-		case IR::Opcode::AtomicUMinU32:
-		case IR::Opcode::AtomicSMaxI32:
-		case IR::Opcode::AtomicUMaxU32:
-		case IR::Opcode::AtomicAndU32:
-		case IR::Opcode::AtomicOrU32:
-		case IR::Opcode::AtomicXorU32:
-		case IR::Opcode::AtomicFMinF32:
-		case IR::Opcode::AtomicFMaxF32:
-		case IR::Opcode::FlatLoadUbyte:
-		case IR::Opcode::FlatLoadSbyte:
-		case IR::Opcode::FlatLoadUshort:
-		case IR::Opcode::FlatLoadSshort:
-		case IR::Opcode::FlatLoadDword:
-		case IR::Opcode::FlatStoreByte:
-		case IR::Opcode::FlatStoreShort:
-		case IR::Opcode::FlatStoreDword:
-		case IR::Opcode::DsReadUbyte:
-		case IR::Opcode::DsReadSbyte:
-		case IR::Opcode::DsReadUshort:
-		case IR::Opcode::DsReadSshort:
-		case IR::Opcode::DsReadB32:
-		case IR::Opcode::DsMinF32:
-		case IR::Opcode::DsMaxF32:
-		case IR::Opcode::DsWriteByte:
-		case IR::Opcode::DsWriteShort:
-		case IR::Opcode::DsWriteB32:
-		case IR::Opcode::DsWriteAddtidB32:
-		case IR::Opcode::DsReadAddtidB32:
-		case IR::Opcode::DsAppend:
-		case IR::Opcode::DsConsume:
-		case IR::Opcode::ImageLoad:
-		case IR::Opcode::ImageStore:
-		case IR::Opcode::ImageSample:
-		case IR::Opcode::ImageGather4:
-		case IR::Opcode::Export: return true;
-		default: return false;
-	}
-}
-
-bool NeedsDispatcherForStructuredLoopHeader(const IR::Program& ir, uint32_t* failure_block,
-                                            std::string* reason) {
-	for (const auto& block: ir.blocks) {
-		if (!block.terminator.loop_header) {
-			continue;
-		}
-		for (const auto& inst: block.instructions) {
-			if (!InstructionMaySplitSpirvBlock(inst)) {
-				continue;
-			}
-			if (reason != nullptr) {
-				*reason = fmt::format("loop header block {} contains an instruction whose SPIR-V "
-				                      "lowering emits internal control flow: {}",
-				                      block.id, IR::InstructionToString(inst).c_str());
-			}
-			if (failure_block != nullptr) {
-				*failure_block = block.id;
-			}
-			return true;
-		}
-	}
-	return false;
-}
-
 enum class EmbeddedFetchValueType {
 	Unknown,
 	Constant,
@@ -771,69 +692,6 @@ bool TryRecompile(std::span<const uint32_t> code, const CompileOptions& options,
 		return false;
 	}
 
-	if (!dispatcher_fallback) {
-		const auto select_dispatcher = [&](uint32_t failure_block, const std::string& reason,
-		                                   const CFG::Graph& diagnostic_cfg) {
-			dispatcher_fallback    = true;
-			dispatcher_reason      = reason;
-			cfg.failure_kind       = CFG::FailureKind::StructuredControlFlow;
-			cfg.failure_block      = failure_block;
-			cfg.unsupported_reason = reason;
-			ir.cfg_failure_kind    = CFG::FailureKind::StructuredControlFlow;
-			ir.fallback_reason     = reason;
-			LogDispatcherFallback(options, diagnostic_cfg, "emit", reason);
-		};
-		const auto retry_budget = cfg.natural_loops.size();
-		for (size_t retry = 0;; retry++) {
-			std::string emitter_reason;
-			uint32_t    failure_block = UINT32_MAX;
-			if (!NeedsDispatcherForStructuredLoopHeader(ir, &failure_block, &emitter_reason)) {
-				break;
-			}
-			if (retry >= retry_budget) {
-				select_dispatcher(failure_block,
-				                  emitter_reason + "; no-clone loop-header isolation exhausted",
-				                  cfg);
-				break;
-			}
-
-			const auto* source_block = cfg.FindBlock(failure_block);
-			const auto  source_pc = source_block != nullptr ? source_block->start_pc : UINT32_MAX;
-			const auto  previous_cfg  = cfg;
-			const auto  blocks_before = cfg.blocks.size();
-			std::string isolation_error;
-			if (!CFG::IsolateLoopHeader(cfg, failure_block, &isolation_error)) {
-				const auto failed_cfg = cfg;
-				cfg                   = previous_cfg;
-				select_dispatcher(failure_block,
-				                  fmt::format("{}; no-clone loop-header isolation failed: {}",
-				                              emitter_reason, isolation_error),
-				                  failed_cfg);
-				break;
-			}
-
-			IR::Program isolated_ir;
-			if (!lower_program(isolated_ir)) {
-				const auto lower_error = error != nullptr ? *error : "IR lowering failed";
-				if (error != nullptr) {
-					error->clear();
-				}
-				cfg = previous_cfg;
-				select_dispatcher(failure_block,
-				                  fmt::format("{}; isolated loop-header lowering failed: {}",
-				                              emitter_reason, lower_error),
-				                  cfg);
-				break;
-			}
-			ir = std::move(isolated_ir);
-			LOGF("%s CFG isolated semantic loop header: stage=%s hash=0x%016" PRIx64
-			     " source_block=%" PRIu32 " pc=0x%08" PRIx32 " blocks=%" PRIu64 "->%" PRIu64
-			     " reason=%s\n",
-			     GetDumpLabel(options), StageName(options.stage), options.shader_hash,
-			     failure_block, source_pc, static_cast<uint64_t>(blocks_before),
-			     static_cast<uint64_t>(cfg.blocks.size()), emitter_reason.c_str());
-		}
-	}
 	LOGF("%s phase end: stage=%s hash=0x%016" PRIx64 " IR LowerProgram blocks=%" PRIu64
 	     " elapsed_ms=%" PRIu64 "\n",
 	     GetDumpLabel(options), StageName(options.stage), options.shader_hash,

src/graphics/shader/recompiler/frontend/cfg/ShaderCFG.cpp

@@ -1264,6 +1264,11 @@ bool IsInnermostLoopControlConditional(const Graph& graph, const BasicBlock& blo
 		};
 		return is_repeat_target(true_target) && is_repeat_target(false_target);
 	}
+	const bool true_in_body  = Contains(loop->body_blocks, true_target);
+	const bool false_in_body = Contains(loop->body_blocks, false_target);
+	if (true_in_body != false_in_body) {
+		return true;
+	}
 	const auto is_control_target = [&](uint32_t target) {
 		return target == loop->merge || target == loop->continue_block;
 	};
@@ -1342,6 +1347,37 @@ bool CanonicalizeNaturalLoops(Graph& graph, std::string* error) {
 	return false;
 }
 
+bool IsolateSemanticLoopHeaders(Graph& graph, std::string* error) {
+	const auto isolation_budget = graph.natural_loops.size() + 1u;
+	for (size_t isolation = 0; isolation < isolation_budget; isolation++) {
+		const auto loop = std::find_if(graph.natural_loops.begin(), graph.natural_loops.end(),
+		                               [&](const auto& value) {
+			                               const auto* header = graph.FindBlock(value.header);
+			                               return header != nullptr &&
+			                                      header->inst_begin != header->inst_end;
+		                               });
+		if (loop == graph.natural_loops.end()) {
+			return true;
+		}
+
+		// SPIR-V requires OpLoopMerge and its branch to remain in the loop header's
+		// physical block. Keep that header as a dedicated control node, exactly like
+		// the separate Loop node in shadPS4's structured AST. Guest instructions live
+		// in the body so later lowering may introduce bounds/EXEC control flow without
+		// displacing OpLoopMerge into a helper-created block.
+		if (!IsolateSemanticLoopHeader(graph, loop->header)) {
+			SetFailure(graph, FailureKind::StructuredControlFlow, loop->header,
+			           fmt::format("failed to isolate semantic loop header {}", loop->header),
+			           error);
+			return false;
+		}
+	}
+
+	SetFailure(graph, FailureKind::StructuredControlFlow, graph.entry_block,
+	           "CFG semantic loop-header isolation exceeded rewrite budget", error);
+	return false;
+}
+
 bool SplitSharedMergeBlock(Graph& graph, uint32_t merge,
                            const std::vector<uint32_t>& construct_blocks,
                            bool                         force_split = false) {
@@ -2113,6 +2149,9 @@ bool StructurizeImpl(Graph& graph, std::string* error) {
 	if (!SplitSharedMergeBlocks(graph, error)) {
 		return false;
 	}
+	if (!IsolateSemanticLoopHeaders(graph, error)) {
+		return false;
+	}
 	ClearStructuredTerminators(graph);
 
 	std::map<uint32_t, uint32_t> merge_headers;
@@ -2139,6 +2178,14 @@ bool StructurizeImpl(Graph& graph, std::string* error) {
 			    error);
 			return false;
 		}
+		if (header->inst_begin != header->inst_end ||
+		    header->terminator.kind != TerminatorKind::Branch) {
+			SetFailure(graph, FailureKind::StructuredControlFlow, loop.header,
+			           fmt::format("loop header {} is not a dedicated empty control block",
+			                       loop.header),
+			           error);
+			return false;
+		}
 		if (!reserve_merge_block(loop.header, loop.merge)) {
 			return false;
 		}
@@ -2207,31 +2254,6 @@ bool Structurize(Graph& graph, std::string* error) {
 	return false;
 }
 
-bool IsolateLoopHeader(Graph& graph, uint32_t header_id, std::string* error) {
-	const auto loop =
-	    std::find_if(graph.natural_loops.begin(), graph.natural_loops.end(),
-	                 [header_id](const auto& value) { return value.header == header_id; });
-	if (loop == graph.natural_loops.end()) {
-		SetFailure(graph, FailureKind::StructuredControlFlow, header_id,
-		           fmt::format("block {} is not a natural loop header", header_id), error);
-		return false;
-	}
-
-	ClearStructuredTerminators(graph);
-	if (!IsolateSemanticLoopHeader(graph, header_id)) {
-		SetFailure(graph, FailureKind::StructuredControlFlow, header_id,
-		           fmt::format("loop header block {} has no semantic instructions", header_id),
-		           error);
-		return false;
-	}
-
-	graph.unsupported   = false;
-	graph.failure_kind  = FailureKind::None;
-	graph.failure_block = UINT32_MAX;
-	graph.unsupported_reason.clear();
-	return Structurize(graph, error);
-}
-
 std::string BranchConditionToString(BranchCondition condition) {
 	switch (condition) {
 		case BranchCondition::Always: return "always";

src/graphics/shader/recompiler/frontend/cfg/ShaderCFG.h

@@ -110,7 +110,6 @@ struct Graph {
 
 bool        BuildGraph(const Decoder::Program& program, Graph& graph, std::string* error);
 bool        Structurize(Graph& graph, std::string* error);
-bool        IsolateLoopHeader(Graph& graph, uint32_t header_id, std::string* error);
 std::string BranchConditionToString(BranchCondition condition);
 std::string FailureKindToString(FailureKind kind);
 std::string GraphToString(const Graph& graph);

tests/shaderCfgTests.cpp

@@ -6957,11 +6957,8 @@ void TestNewShaderRecompilerCfgLoopHeaderDsAppendConsumeStructured() {
       CfgInstructionCoverage(graph, decoded.instructions.size());
   Check(ShaderRecompiler::CFG::Structurize(graph, &error), error.c_str());
   Check(graph.natural_loops.size() == 1u, "DS loop was not preserved");
-  Check(ShaderRecompiler::CFG::IsolateLoopHeader(
-            graph, graph.natural_loops.front().header, &error),
-        error.c_str());
   Check(graph.blocks.size() == original_block_count + 1u,
-        "DS loop did not add exactly one empty header");
+        "DS loop structurization did not add exactly one empty header");
   Check(CfgInstructionCoverage(graph, decoded.instructions.size()) ==
             original_coverage,
         "DS loop canonicalization duplicated semantic instructions");
@@ -7018,6 +7015,36 @@ void TestNewShaderRecompilerCfgLoopHeaderDsReadStructured() {
   CheckSpirvBinaryValidates(result.spirv);
 }
 
+void TestNewShaderRecompilerCfgLoopHeaderDsRead2B64Structured() {
+  const uint32_t shader[] = {
+      EncodeDs0(0x77, (48u << 8u) | 32u), // loop: ds_read2_b64 v[0:3], v1
+      EncodeDs1(0, 0, 1),
+      EncodeSop2(0x00, 2, 2, 129), // s_add_u32 s2, s2, 1
+      EncodeSopc(0x0a, 2, 130),    // s_cmp_lt_u32 s2, 2
+      EncodeSopp(0x05, 0xfffbu),   // s_cbranch_scc1 loop
+      EncodeMubuf0(0x1e),
+      EncodeMubuf1(0, 4, 1), // keep all four guarded LDS results live
+      0xbf810000u,
+  };
+
+  auto options = MakeCompileOptions(ShaderType::Compute);
+  options.dump_ir = true;
+
+  ShaderRecompiler::CompileResult result;
+  std::string error;
+  Check(ShaderRecompiler::TryRecompile(shader, options, result, &error),
+        error.c_str());
+  Check(Common::ContainsStr(result.ir_dump, "mode=structured") &&
+            !result.program.dispatcher_fallback,
+        "DS read2 b64 loop header did not stay structured");
+  Check(Common::ContainsStr(result.decoded_dump, "DS_READ2_B64"),
+        "DS read2 b64 loop regression did not decode the captured opcode");
+  Check(!SpirvContainsOpcode(result.spirv, 251),
+        "DS read2 b64 structured SPIR-V unexpectedly contains OpSwitch");
+  CheckSpirvPhiParents(result.spirv);
+  CheckSpirvBinaryValidates(result.spirv);
+}
+
 void TestNewShaderRecompilerCfgSharedOuterAndLoopMerge() {
   const uint32_t shader[] = {
       EncodeSopc(0x06, 0, 0),      // s_cmp_eq_u32 s0, s0
@@ -7439,8 +7466,8 @@ void TestNewShaderRecompilerCfgMultipleLoopLatches() {
   Check(graph.back_edges.size() == 2u,
         "multiple-latch fixture lacks two native backedges");
   Check(ShaderRecompiler::CFG::Structurize(graph, &error), error.c_str());
-  Check(graph.blocks.size() == original_block_count + 1u,
+  Check(graph.blocks.size() == original_block_count + 2u,
-        "multiple native latches did not create one synthetic continue");
+        "multiple native latches did not create one synthetic continue and one empty header");
   Check(graph.back_edges.size() == 1u && graph.natural_loops.size() == 1u,
         "multiple native latches were not coalesced to one SPIR-V backedge");
   const auto &loop = graph.natural_loops.front();
@@ -7527,13 +7554,13 @@ void TestNewShaderRecompilerCfgNestedEarlyExitLoopForwarders() {
         "nested early-exit fixture has the wrong native CFG");
   const bool structured = ShaderRecompiler::CFG::Structurize(graph, &error);
   Check(structured, error.c_str());
-  Check(graph.blocks.size() == original_block_count + 3u &&
+  Check(graph.blocks.size() == original_block_count + 4u &&
             CfgInstructionCoverage(graph, decoded.instructions.size()) ==
                 original_coverage &&
             std::ranges::count_if(graph.blocks,
                                   [](const auto &block) {
                                     return block.inst_begin == block.inst_end;
-                                  }) == original_empty_blocks + 3,
+                                  }) == original_empty_blocks + 4,
         "nested early-exit structurization changed semantic coverage");
   const auto *preheader = graph.FindBlockByPc(0x00u);
   const auto *outer = graph.FindBlockByPc(0x08u);
@@ -11213,12 +11240,12 @@ void TestNewShaderRecompilerSpirvSizeBaselines() {
       EncodeSopp(0x01),
   };
   const auto structured_result = compile("structured-phi", structured_phi,
-                                         {.words = 136,
+                                         {.words = 140,
-                                          .instructions = 39,
+                                          .instructions = 41,
                                           .phis = 1,
-                                          .labels = 7,
+                                          .labels = 8,
                                           .loop_merges = 1,
-                                          .branches = 5,
+                                          .branches = 6,
                                           .conditional_branches = 1});
   const auto structured_metrics = MeasureSpirv(structured_result.spirv);
   Check(Common::ContainsStr(structured_result.ir_dump, "Phi"),
@@ -11230,12 +11257,12 @@ void TestNewShaderRecompilerSpirvSizeBaselines() {
   CheckSpirvPhiParents(structured_result.spirv);
   const auto structured_repeat =
       compile("structured-phi-repeat", structured_phi,
-              {.words = 136,
+              {.words = 140,
-               .instructions = 39,
+               .instructions = 41,
                .phis = 1,
-               .labels = 7,
+               .labels = 8,
                .loop_merges = 1,
-               .branches = 5,
+               .branches = 6,
                .conditional_branches = 1});
   Check(structured_repeat.spirv == structured_result.spirv,
         "deferred Phi patching is not deterministic");
@@ -11467,6 +11494,7 @@ int main() {
   TestNewShaderRecompilerCfgLoopHeaderBufferLoadDispatcher();
   TestNewShaderRecompilerCfgLoopHeaderDsAppendConsumeStructured();
   TestNewShaderRecompilerCfgLoopHeaderDsReadStructured();
+  TestNewShaderRecompilerCfgLoopHeaderDsRead2B64Structured();
   TestNewShaderRecompilerCfgSharedOuterAndLoopMerge();
   TestNewShaderRecompilerCfgLoopEarlyBreakNoSelection();
   TestNewShaderRecompilerCfgNestedLoopNonlocalExitDispatcher();