perf(shader): avoid redundancy traversal map copies

core/libs/prx/libSceAgcDriver/tests/SpirvOptimizer.cpp

@@ -378,6 +378,77 @@ OpFunctionEnd
     }
 }
 
+void testLongControlFlow(bool optimizationEnabled, bool large) {
+    const char* mode = std::getenv("APS5_SPIRV_OPT");
+    const bool full = mode != nullptr && std::string(mode) == "full";
+    const std::size_t blocks = large && optimizationEnabled && !full ? 512 : 8;
+    std::string source = R"(OpCapability Shader
+OpCapability StorageBuffer8BitAccess
+OpExtension "SPV_KHR_8bit_storage"
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %array ArrayStride 4
+OpMemberDecorate %buffer 0 Offset 0
+OpDecorate %buffer Block
+OpDecorate %data DescriptorSet 0
+OpDecorate %data Binding 0
+OpDecorate %byteArray ArrayStride 1
+OpMemberDecorate %byteBuffer 0 Offset 0
+OpDecorate %byteBuffer Block
+OpDecorate %bytes DescriptorSet 0
+OpDecorate %bytes Binding 1
+%void = OpTypeVoid
+%uint = OpTypeInt 32 0
+%boolean = OpTypeBool
+%byte = OpTypeInt 8 0
+%byteArray = OpTypeRuntimeArray %byte
+%byteBuffer = OpTypeStruct %byteArray
+%byteBufferPointer = OpTypePointer StorageBuffer %byteBuffer
+%bytePointer = OpTypePointer StorageBuffer %byte
+%array = OpTypeRuntimeArray %uint
+%buffer = OpTypeStruct %array
+%bufferPointer = OpTypePointer StorageBuffer %buffer
+%valuePointer = OpTypePointer StorageBuffer %uint
+%function = OpTypeFunction %void
+%data = OpVariable %bufferPointer StorageBuffer
+%bytes = OpVariable %byteBufferPointer StorageBuffer
+%zero = OpConstant %uint 0
+%one = OpConstant %uint 1
+%two = OpConstant %uint 2
+)";
+    for (std::size_t i = 0; i < blocks * 4; ++i) source += "%index" + std::to_string(i) + " = OpConstant %uint " + std::to_string(i + 1) + "\n";
+    source += "%main = OpFunction %void None %function\n%entry = OpLabel\n%input = OpAccessChain %valuePointer %data %zero %zero\n%seed = OpLoad %uint %input Volatile\n%condition = OpINotEqual %boolean %seed %zero\nOpBranch %block0\n";
+    for (std::size_t i = 0; i < blocks; ++i) {
+        const auto number = std::to_string(i);
+        const auto previous = i == 0 ? "%seed" : "%next" + std::to_string(i - 1);
+        source += "%block" + number + " = OpLabel\n%sum" + number + " = OpIAdd %uint " + previous + " %one\n";
+        source += "%duplicate" + number + " = OpIAdd %uint " + previous + " %one\n%shared" + number + " = OpIAdd %uint %seed %one\n";
+        for (std::size_t j = 0; j < 3; ++j) {
+            const auto index = std::to_string(i * 4 + j);
+            source += "%slot" + index + " = OpAccessChain %valuePointer %data %zero %index" + index + "\n";
+            source += "OpStore %slot" + index + " %" + (j == 0 ? "sum" : j == 1 ? "duplicate" : "shared") + number + " Volatile\n";
+        }
+        const auto branchIndex = std::to_string(i * 4 + 3);
+        source += "%branchSlot" + number + " = OpAccessChain %valuePointer %data %zero %index" + branchIndex + "\n";
+        source += "OpSelectionMerge %join" + number + " None\nOpBranchConditional %condition %left" + number + " %right" + number + "\n";
+        source += "%left" + number + " = OpLabel\n%leftValue" + number + " = OpIAdd %uint %sum" + number + " %one\nOpStore %branchSlot" + number + " %leftValue" + number + " Volatile\nOpBranch %join" + number + "\n";
+        source += "%right" + number + " = OpLabel\n%rightCopy" + number + " = OpIAdd %uint %sum" + number + " %one\nOpStore %branchSlot" + number + " %rightCopy" + number + " Volatile\n";
+        source += "%rightValue" + number + " = OpIAdd %uint %sum" + number + " %two\nOpStore %branchSlot" + number + " %rightValue" + number + " Volatile\nOpBranch %join" + number + "\n";
+        source += "%join" + number + " = OpLabel\n%next" + number + " = OpPhi %uint %leftValue" + number + " %left" + number + " %rightValue" + number + " %right" + number + "\n";
+        if (i + 1 == blocks) source += "%byteSlot = OpAccessChain %bytePointer %bytes %zero %zero\n%truncated = OpUConvert %byte %next" + number + "\nOpStore %byteSlot %truncated Volatile\nOpReturn\n";
+        else source += "OpBranch %block" + std::to_string(i + 1) + "\n";
+    }
+    source += "OpFunctionEnd\n";
+    const auto input = assemble(source);
+    const auto output = ShaderRecompiler::ValidateAndOptimizeSpirv(input, Vulkan11, Spirv13, false);
+    const auto stores = opcodeCount(output, spv::OpStore);
+    check(stores >= blocks * 4 + 1 && stores <= blocks * 6 + 1, "long control-flow optimization discarded observable stores");
+    const auto adds = opcodeCount(output, spv::OpIAdd);
+    if (!optimizationEnabled) check(output == input, "disabled optimization changed the long control-flow module");
+    else check(adds >= blocks && adds <= blocks * 4, "control-flow optimization lost arithmetic or retained redundant values");
+}
+
 }
 
 int main() {
@@ -388,6 +459,8 @@ int main() {
     testInvalidInputIsRejected();
     testSpecializedSelectionExit();
     testSpecializedDeadContinue();
+    testLongControlFlow(optimizationEnabled, false);
+    testLongControlFlow(optimizationEnabled, true);
     if (failures != 0) {
         std::fprintf(stderr, "%d optimizer check(s) failed\n", failures);
         return 1;

core/shader/recompiler/PatchSpirvTools.cmake

@@ -3,31 +3,101 @@ set(spirvToolsRedundancySource "${spirvToolsSourceDir}/redundancy_elimination.cp
 set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${spirvToolsRedundancySource}")
 file(READ "${spirvToolsRedundancySource}" spirvToolsRedundancy)
 
+set(spirvToolsLocalRedundancySource "${spirvToolsSourceDir}/local_redundancy_elimination.cpp")
+set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${spirvToolsLocalRedundancySource}")
+file(READ "${spirvToolsLocalRedundancySource}" spirvToolsLocalRedundancy)
+set(spirvToolsExpectedElimination [=[
+  auto func = [this, &vnTable, &modified, value_to_ids](Instruction* inst) {
+    if (inst->result_id() == 0) {
+      return;
+    }
+
+    uint32_t value = vnTable.GetValueNumber(inst);
+
+    if (value == 0) {
+      return;
+    }
+
+    auto candidate = value_to_ids->insert({value, inst->result_id()});
+    if (!candidate.second) {
+      context()->KillNamesAndDecorates(inst);
+      context()->ReplaceAllUsesWith(inst->result_id(), candidate.first->second);
+      context()->KillInst(inst);
+      modified = true;
+    }
+  };
+  block->ForEachInst(func);
+]=])
+string(FIND "${spirvToolsLocalRedundancy}" "${spirvToolsExpectedElimination}" spirvToolsEliminationPosition)
+if(spirvToolsEliminationPosition EQUAL -1)
+    message(FATAL_ERROR "SPIRV-Tools value elimination has changed; review the local traversal fix")
+endif()
+
 set(spirvToolsOldTraversal [=[
+  struct State {
+    DominatorTreeNode* node;
+    std::map<uint32_t, uint32_t> value_to_id_map;
+  };
+  std::vector<State> todo;
+  todo.push_back({bb, std::map<uint32_t, uint32_t>()});
+  bool modified = false;
+  for (size_t next_node = 0; next_node < todo.size(); next_node++) {
+    modified |= EliminateRedundanciesInBB(todo[next_node].node->bb_, vnTable,
+                                          &todo[next_node].value_to_id_map);
     for (DominatorTreeNode* child : todo[next_node].node->children_) {
       todo.push_back({child, todo[next_node].value_to_id_map});
     }
+  }
 ]=])
 set(spirvToolsNewTraversal [=[
-    const auto& children = todo[next_node].node->children_;
-    for (size_t child_index = 0; child_index < children.size(); ++child_index) {
-      if (child_index + 1 == children.size()) {
-        todo.push_back({children[child_index],
-                        std::move(todo[next_node].value_to_id_map)});
-      } else {
-        todo.push_back({children[child_index], todo[next_node].value_to_id_map});
-      }
+  struct State {
+    DominatorTreeNode* node;
+    size_t next_child;
+    size_t added_begin;
+    bool entered;
+  };
+  std::map<uint32_t, uint32_t> value_to_id_map;
+  std::vector<uint32_t> added_values;
+  std::vector<State> todo;
+  todo.push_back({bb, 0, 0, false});
+  bool modified = false;
+  while (!todo.empty()) {
+    auto& state = todo.back();
+    if (!state.entered) {
+      state.entered = true;
+      state.node->bb_->ForEachInst([&](Instruction* inst) {
+        if (inst->result_id() == 0) return;
+        const uint32_t value = vnTable.GetValueNumber(inst);
+        if (value == 0) return;
+        auto candidate = value_to_id_map.insert({value, inst->result_id()});
+        if (candidate.second) {
+          added_values.push_back(value);
+        } else {
+          context()->KillNamesAndDecorates(inst);
+          context()->ReplaceAllUsesWith(inst->result_id(), candidate.first->second);
+          context()->KillInst(inst);
+          modified = true;
+        }
+      });
+    }
+    const auto& children = state.node->children_;
+    if (state.next_child != children.size()) {
+      auto* child = children[state.next_child++];
+      todo.push_back({child, 0, added_values.size(), false});
+      continue;
+    }
+    while (added_values.size() != state.added_begin) {
+      value_to_id_map.erase(added_values.back());
+      added_values.pop_back();
     }
-    todo[next_node].value_to_id_map.clear();
+    todo.pop_back();
+  }
 ]=])
 string(FIND "${spirvToolsRedundancy}" "${spirvToolsOldTraversal}" spirvToolsTraversalPosition)
 if(spirvToolsTraversalPosition EQUAL -1)
     message(FATAL_ERROR "SPIRV-Tools redundancy traversal has changed; review the local memory fix")
 endif()
 string(REPLACE "${spirvToolsOldTraversal}" "${spirvToolsNewTraversal}" spirvToolsRedundancy "${spirvToolsRedundancy}")
-string(REPLACE "#include \"source/opt/redundancy_elimination.h\"\n"
-        "#include \"source/opt/redundancy_elimination.h\"\n\n#include <utility>\n"
-        spirvToolsRedundancy "${spirvToolsRedundancy}")
 
 set(spirvToolsPatchedDir "${CMAKE_BINARY_DIR}/3rdparty/SPIRV-Tools/patched")
 set(spirvToolsPatchedSource "${spirvToolsPatchedDir}/redundancy_elimination.cpp")

docs/dev/TechnicalDebt.md

@@ -4,7 +4,6 @@ Entries about one library or component are in [debt](debt/), one file per librar
 
 ### Build
 
-- The bundled [SPIRV-Tools build](../../core/shader/recompiler/PatchSpirvTools.cmake) replaces the redundancy-elimination source with a build-directory copy that releases processed traversal maps. Remove this patch when the pinned revision includes [SPIRV-Tools #6962](https://github.com/KhronosGroup/SPIRV-Tools/pull/6962). System-dependency builds need the fix in their installed SPIRV-Tools package.
 - Building on Windows requires a specific version of mingw - MinGW-w64 GCC 15.2.0 (`winlibs-gcc15`, `x86_64-ucrt-posix-seh`)
 - Even compiled prx libraries on Windows require nearby (static linking of these dependencies causes conflicts):
   - libgcc_s_seh-1.dll

docs/dev/debt/shader.md

@@ -4,6 +4,8 @@
 
 - winpthreads initializes a statically initialized rwlock on its first lock and fails a concurrent first lock with `EINVAL`, which `std::shared_mutex` and `std::shared_timed_mutex` ignore, so that lock does not exclude. The host `std::shared_mutex` objects in [Recompiler.cpp](../../../core/shader/recompiler/Recompiler.cpp) are locked once when created. A new host `std::shared_mutex` used on Windows needs the same.
 
+- The bundled [SPIRV-Tools build](../../../core/shader/recompiler/PatchSpirvTools.cmake) replaces the redundancy-elimination source with a build-directory copy. A depth-first traversal keeps one value map and removes each subtree's added values on return, avoiding growing map copies and preventing sibling values from leaking into one another. This extends the processed-map release fix in [SPIRV-Tools #6962](https://github.com/KhronosGroup/SPIRV-Tools/pull/6962). Remove the patch when the pinned dependency has equivalent copy-free traversal; #6962 alone still copies maps for sibling branches. System-dependency builds need an equivalent fix in their installed package. Optimization passes and validation remain unchanged; wider shader and game performance measurement is needed.
+
 ### Unknown function info
 
 - [Packed pixel ancillary word](../../../core/shader/recompiler/Optimization/src/ConstantFolder.cpp) (shader recompiler) - the sample index at bits 8-11 and the render target index at bits 16-28 follow #295. The sample index is the SampleId builtin, an equality inferred, not measured. The render target index is the Layer builtin when the draw exports it (a layered draw, `PA_CL_VS_OUT_CNTL` bit 18; the pipeline specialization constant `LayerExported`) and 0 otherwise, the first layer Vulkan renders when no pre-rasterization stage writes Layer. Reading the unwritten Layer builtin instead makes AMD's Windows driver drop the fragment's writes (#4800); with the constant at 0 the load is folded away. Mesa reads both fields from those bits, with the layer 14 bits wide only from GFX12 on ([ac_nir_lower_intrinsics_to_args.c](https://gitlab.freedesktop.org/mesa/mesa/-/blob/bffd91947efc1932da99aca69fc540d6d4201b0c/src/amd/common/nir/ac_nir_lower_intrinsics_to_args.c#L342-L368)). A value passed through a select, move or phi whose every read is an extract inside one of those fields is rebuilt from the two fields alone. Any other read the masked-select pass cannot drop gets the whole word: the primitive class at bits 0-1 (0 point, 1 line, 2 triangle, 3 rectangle), which the draw sets as a pipeline specialization constant ([RasterPrimitiveOf](../../../core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp)) in the shaders that read it, plus the sample field, with every other bit zero. That layout comes from the RDNA 3 and 3.5 ISA guides (prim type 1:0, VRS rates 3:2 and 5:4, eye id 7, sample 11:8, render target index 28:16). The GCN register guides give the same prim type and sample fields and put the render target index at 26:16, so the two sources agree for layers below 2048. AMD's LLPC reads the primitive class from bits 1:0 with the same encoding ([LowerGlCompatibility.cpp](https://github.com/GPUOpen-Drivers/llpc/blob/40cb8d95ad8d6f7f1652e3fd47d39667594cce08/llpc/lowering/LowerGlCompatibility.cpp#L830-L836)) and without a GFX-level check ([getPrimType](https://github.com/GPUOpen-Drivers/llpc/blob/40cb8d95ad8d6f7f1652e3fd47d39667594cce08/lgc/lowering/LowerInOut.cpp#L5307-L5315)). Nobody has checked it on gfx1013. In the whole word only bits 0-1 and 8-11 have a source. Bits 2-7, 12-15 and 29-31 are assumed zero, not measured: the VRS rates (2-5) and the eye id (7) because the driver supports neither variable-rate shading nor multiview, so a title that enables either would read other bits on the console, and bits 6, 12-15 and 29-31 because no source gives them a meaning. The sample field reads SampleId, which is always 0 here because the driver rejects MSAA and per-sample shading (`PA_SC_AA_CONFIG`, `PS_ITER_SAMPLE`). The whole word has the same 0 at bits 16-28. A change that makes draws export a layer must restore the Layer read in both places