From 6a8bf4c03cc2b933adf505f81cc142edf58ca272 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 22 Aug 2026 21:45:13 -0400 Subject: [PATCH] [Fix, Test] (DirectVulkan): resolve a lowered atomic-counter block from the atomic-counter binding points --- .../DirectVulkan/Renderer/UniformManager.cpp | 41 +++++++++++++++---- .../Scenarios/AtomicCounterScenario.cpp | 11 ----- MobileGL/MG_Util/ShaderTranspiler/Types.h | 22 ++++++++++ 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index f01a29f7..bd0c42d5 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -17,6 +17,7 @@ #include "MG_Util/Converters/MGToStr/FramebufferEnumConverter.h" #include "MG_Util/Converters/MGToVk/TextureEnumConverter.h" #include "MG_Util/Metrics/TextureMetrics.h" +#include "MG_Util/ShaderTranspiler/Types.h" #include #include #include @@ -923,22 +924,46 @@ namespace MobileGL::MG_Backend::DirectVulkan { const Int blockIndex = programObj.storageBlockIndexByBinding[binding]; MOBILEGL_ASSERT(blockIndex >= 0, "ResolveStorageBufferDescriptor: no SSBO block mapped to binding %u", binding); + // An atomic counter is not an SSBO the application ever declared: glslang lowers every + // atomic_uint onto a synthesized gl_AtomicCounterBlock_ storage block, where N is the + // GL ATOMIC-COUNTER binding. That block arrives here auto-mapped to an arbitrary + // storage-block slot, so resolving it the SSBO way looked up GL_SHADER_STORAGE_BUFFER + // point N' - which is never where glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, N, ...) put + // the buffer. The counter therefore never reached the shader (KHR-GL43 + // shader_atomic_counters.advanced-usage-*), and when the application also bound an SSBO at + // the colliding slot the descriptor silently aliased it, so the dispatch wrote over the + // application's own buffer. DirectGLES has always taken this branch explicitly + // (SyncAtomicCounterBuffers); this is the same rule in Magma's descriptor resolution. + // + // Only the SOURCE of the handle differs. The per-counter layout(offset=) is already folded + // into the block's SPIR-V member offsets on this path (FlattenAtomicCounterBlockPass is + // DirectGLES-only), so everything below - residency, the glBindBufferRange window, the + // descriptor fill - is target-agnostic and stays exactly as it was. + const String& blockName = programObj.storageBlockNameByBinding[binding]; + const Int atomicCounterBinding = MG_Util::ShaderTranspiler::AtomicCounterBlockGlBinding(blockName); + const Bool isAtomicCounterBlock = atomicCounterBinding >= 0; + const BufferTarget bufferTarget = + isAtomicCounterBlock ? BufferTarget::AtomicCounter : BufferTarget::ShaderStorage; // A block instance array declares one block whose elements take consecutive GL binding // points from the declared one (GL 4.6 core 7.8), and the reflection collapses the whole - // array to that one block - so the element index IS the offset from its binding. + // array to that one block - so the element index IS the offset from its binding. glslang + // synthesizes one counter block per GL binding, so a counter block is never an instance + // array and `element` is always 0 there; the +element rule stays with the SSBO case. const GLuint frontendBinding = - GetShaderStorageBlockBinding(program, static_cast(blockIndex)) + element; + isAtomicCounterBlock + ? static_cast(atomicCounterBinding) + : GetShaderStorageBlockBinding(program, static_cast(blockIndex)) + element; const Uint32 bindingPointCount = - static_cast(MG_State::pGLContext->GetBufferBindingPointCount(BufferTarget::ShaderStorage)); + static_cast(MG_State::pGLContext->GetBufferBindingPointCount(bufferTarget)); MOBILEGL_ASSERT(frontendBinding < bindingPointCount, - "ResolveStorageBufferDescriptor: frontend SSBO binding %u out of range for block '%s'", - frontendBinding, programObj.storageBlockNameByBinding[binding].c_str()); + "ResolveStorageBufferDescriptor: frontend binding %u out of range for block '%s'", + frontendBinding, blockName.c_str()); - auto& bindingPoint = MG_State::pGLContext->GetBufferBindingPoint(BufferTarget::ShaderStorage, frontendBinding); + auto& bindingPoint = MG_State::pGLContext->GetBufferBindingPoint(bufferTarget, frontendBinding); const auto& bufferObject = bindingPoint.GetBoundObject(); if (bufferObject == nullptr) { - MGLOG_E_ONCE("ResolveStorageBufferDescriptor: no SSBO bound at frontend binding %u for block '%s'", - frontendBinding, programObj.storageBlockNameByBinding[binding].c_str()); + MGLOG_E_ONCE("ResolveStorageBufferDescriptor: no buffer bound at frontend binding %u for block '%s'", + frontendBinding, blockName.c_str()); return false; } diff --git a/MobileGL/MG_IntegrationTest/Scenarios/AtomicCounterScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/AtomicCounterScenario.cpp index 55884664..7d0de004 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/AtomicCounterScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/AtomicCounterScenario.cpp @@ -87,11 +87,6 @@ void main() { << " and GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS is " << buffers << "; this needs 3 and 2"; } - if (!AtomicCountersAreWired()) { - GTEST_SKIP() << "atomic counter buffers are not wired up on " << Gl().BackendName() - << " yet: glslang lowers them onto a storage block and that block's descriptor " - << "is still resolved from the shader-storage binding points"; - } m_program = CompileComputeProgram(kCounterComputeSource); ASSERT_NE(m_program, 0u) << m_buildLog; } @@ -105,12 +100,6 @@ void main() { m_program = 0; } - // Magma binds the lowered block as an ordinary storage-buffer descriptor resolved - // from GL_SHADER_STORAGE_BUFFER point N, so the counter buffer never reaches it. The - // frontend half (limits, reflection queries, the link-time offset rules) is - // backend-agnostic and is covered by the unit suites; only the VALUE is scoped here. - bool AtomicCountersAreWired() const { return Gl().BackendName() != "DirectVulkan"; } - unsigned int CompileComputeProgram(const char* source) { const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); glShaderSource(shader, 1, &source, nullptr); diff --git a/MobileGL/MG_Util/ShaderTranspiler/Types.h b/MobileGL/MG_Util/ShaderTranspiler/Types.h index 13c628be..0b7dc74b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/Types.h +++ b/MobileGL/MG_Util/ShaderTranspiler/Types.h @@ -20,6 +20,28 @@ namespace MobileGL { // buffer, and the trailing number is the only place the GL binding survives. inline constexpr const char* ATOMIC_COUNTER_BLOCK_PREFIX = "gl_AtomicCounterBlock"; + // "gl_AtomicCounterBlock_5" -> 5; -1 for any name that is not one of these blocks. + // Recovering N from the NAME is not a shortcut, it is the only way: the block reaches + // a backend auto-mapped to whatever storage-block slot the IO mapper had free, and + // that number has no relation to the GL atomic-counter binding the application asked + // for (see TMglGlslIoResolver). A backend that resolves the block from the + // shader-storage binding points therefore binds the wrong buffer - or, worse, the + // application's own SSBO at the same slot. + inline Int AtomicCounterBlockGlBinding(StringView name) { + const SizeT prefixLength = StringView(ATOMIC_COUNTER_BLOCK_PREFIX).size(); + // Needs the prefix, the '_' and at least one digit. + if (name.size() <= prefixLength + 1) return -1; + if (name.compare(0, prefixLength, ATOMIC_COUNTER_BLOCK_PREFIX) != 0) return -1; + if (name[prefixLength] != '_') return -1; + Int binding = 0; + for (SizeT i = prefixLength + 1; i < name.size(); ++i) { + if (name[i] < '0' || name[i] > '9') return -1; + binding = binding * 10 + (name[i] - '0'); + if (binding > 0x0FFFFFFF) return -1; // absurd suffix; treat as not-a-counter + } + return binding; + } + // Atomic-counter limits, in ONE place because GL 4.6 requires glGetIntegerv and the // shading language's gl_MaxAtomicCounter* constants to report the same numbers // (KHR-GL43.shader_atomic_counters.basic-glsl-built-in compares them directly).