[Fix, Test] (GLImpl, ShaderTranspiler): reconcile the atomic-counter limits glGetIntegerv and glslang advertise

MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp

@@ -25,6 +25,7 @@
 #include <MG_State/GLState/FramebufferState/FramebufferObject.h>
 #include <MG_Util/Texture/TextureFormatProcessor.h>
 #include <MG_Util/Async/ShaderCompilePool.h>
+#include <MG_Util/ShaderTranspiler/Types.h>
 #include <MG_Backend/BackendObjects.h>
 
 namespace MobileGL::MG_Impl::GLImpl {
@@ -47,12 +48,25 @@ namespace MobileGL::MG_Impl::GLImpl {
         }
 
         constexpr GLint kFrontendMaxComputeUniformComponents = 1024;
-        constexpr GLint kFrontendMaxComputeAtomicCounters = 8;
+        // Every atomic-counter limit is shared with the glslang resource table
-        constexpr GLint kFrontendMaxComputeAtomicCounterBuffers = 8;
+        // (BuildTBuiltInResource) through MG_Util/ShaderTranspiler/Types.h: GL 4.6 requires
+        // glGetIntegerv and the gl_MaxAtomicCounter* built-in constants to agree, and the two
+        // used to be independent tables that disagreed on both the binding count and the buffer
+        // size. Never move one of these without the other.
+        constexpr GLint kFrontendMaxComputeAtomicCounters =
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTERS_PER_STAGE);
+        constexpr GLint kFrontendMaxComputeAtomicCounterBuffers =
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE);
         constexpr GLint kFrontendMaxComputeSharedMemorySize = 32768;
         constexpr GLint kFrontendMaxComputeWorkGroupInvocations = 1024;
-        constexpr GLint kFrontendMaxCombinedAtomicCounters = 8;
+        constexpr GLint kFrontendMaxCombinedAtomicCounters =
-        constexpr GLint kFrontendMaxFragmentAtomicCounters = 8;
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTERS_PER_STAGE);
+        constexpr GLint kFrontendMaxCombinedAtomicCounterBuffers =
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE);
+        constexpr GLint kFrontendMaxFragmentAtomicCounters =
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTERS_PER_STAGE);
+        constexpr GLint kFrontendMaxFragmentAtomicCounterBuffers =
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE);
         constexpr GLint kFrontendMaxGeometryAtomicCounters = 0;
         constexpr GLint kFrontendMaxTessControlAtomicCounters = 0;
         constexpr GLint kFrontendMaxTessEvaluationAtomicCounters = 0;
@@ -66,10 +80,14 @@ namespace MobileGL::MG_Impl::GLImpl {
         constexpr GLint kFrontendMaxTessControlAtomicCounterBuffers = 0;
         constexpr GLint kFrontendMaxTessEvaluationAtomicCounterBuffers = 0;
         constexpr GLint kFrontendMaxVertexAtomicCounterBuffers = 0;
-        // One atomic counter is a uint, and a buffer never has to hold more counters than the
+        // GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS / _SIZE. Both come from the shared table above:
-        // combined limit the frontend advertises. GL 4.6 table 23.63 floors this at 32 bytes.
+        // the binding count is how many counter buffers a backend can actually address (each one
+        // costs a shader-storage binding point once glslang has lowered the counters onto a
+        // storage block), and the size is the byte offset ceiling a counter may be declared at.
+        constexpr GLint kFrontendMaxAtomicCounterBufferBindings =
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTER_BUFFER_BINDINGS);
         constexpr GLint kFrontendMaxAtomicCounterBufferSize =
-            kFrontendMaxCombinedAtomicCounters * static_cast<GLint>(sizeof(GLuint));
+            static_cast<GLint>(MG_Util::ShaderTranspiler::MAX_ATOMIC_COUNTER_BUFFER_SIZE);
         // KHR_debug minima (GL 4.6 table 23.66); the debug entry points are stubs, but the
         // limits they advertise still have to be legal.
         constexpr GLint kFrontendMaxDebugGroupStackDepth = 64;
@@ -1532,6 +1550,9 @@ namespace MobileGL::MG_Impl::GLImpl {
         case GL_MAX_COMBINED_ATOMIC_COUNTERS:
             *params = kFrontendMaxCombinedAtomicCounters;
             return;
+        case GL_MAX_COMBINED_ATOMIC_COUNTER_BUFFERS:
+            *params = kFrontendMaxCombinedAtomicCounterBuffers;
+            return;
         case GL_MAX_COMBINED_UNIFORM_BLOCKS:
             *params = ClampUniformBlockCount(kFrontendMaxCombinedUniformBlocks);
             return;
@@ -1547,6 +1568,9 @@ namespace MobileGL::MG_Impl::GLImpl {
         case GL_MAX_FRAGMENT_ATOMIC_COUNTERS:
             *params = kFrontendMaxFragmentAtomicCounters;
             return;
+        case GL_MAX_FRAGMENT_ATOMIC_COUNTER_BUFFERS:
+            *params = kFrontendMaxFragmentAtomicCounterBuffers;
+            return;
         case GL_MAX_FRAGMENT_SHADER_STORAGE_BLOCKS:
             *params = ClampStorageBlockCount(16); // TODO
             return;
@@ -1984,6 +2008,24 @@ namespace MobileGL::MG_Impl::GLImpl {
         case GL_UNIFORM_BUFFER_START:
             RecordIndexedOnlyGetterError(__func__, pname);
             return;
+        // glBindBufferBase/Range set the GENERIC binding point too (GL 4.6 core 6.1.1), and this
+        // is the one indexed-buffer family whose non-indexed query was never answered - so it
+        // fell through to INVALID_ENUM and left the caller's variable holding whatever was in its
+        // stack slot. _START/_SIZE stay indexed-only, exactly like their uniform-buffer siblings.
+        case GL_ATOMIC_COUNTER_BUFFER_BINDING:
+            if (const auto& obj =
+                    MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::AtomicCounter).GetBoundObject()) {
+                *params = static_cast<GLint>(obj->GetExternalIndex());
+            } else {
+                *params = 0;
+            }
+            return;
+        case GL_ATOMIC_COUNTER_BUFFER_START:
+            RecordIndexedOnlyGetterError(__func__, pname);
+            return;
+        case GL_ATOMIC_COUNTER_BUFFER_SIZE:
+            RecordIndexedOnlyGetterError(__func__, pname);
+            return;
         case GL_UNPACK_ALIGNMENT:
             *params = MG_State::pGLContext->GetPixelStoreParam(PixelStoreParam::UnpackAlignment);
             return;
@@ -2219,18 +2261,20 @@ namespace MobileGL::MG_Impl::GLImpl {
                                                           static_cast<Uint64>(INT32_MAX)));
             break;
         case GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS:
-            *params = static_cast<GLint>(GetIndexedBufferQueryPointCount(BufferTarget::AtomicCounter));
+            // NOT the frontend's binding-point array size (36). A counter buffer only reaches a
+            // shader as a lowered storage block, so the number an implementation can serve is
+            // the reserved shader-storage range, and it has to be the same number glslang
+            // compiles a layout(binding = N) atomic_uint against.
+            *params = std::min<GLint>(kFrontendMaxAtomicCounterBufferBindings,
+                                      static_cast<GLint>(GetIndexedBufferQueryPointCount(BufferTarget::AtomicCounter)));
             break;
         case GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE:
             // The conformance suite splits this evenly across every advertised binding point and
             // binds all of them in one glBindBuffersRange
-            // (KHR-GL44.multi_bind.functional_bind_buffers_range), so the pair has to divide:
+            // (KHR-GL44.multi_bind.functional_bind_buffers_range), so the pair has to divide -
-            // 32 bytes over 36 binding points is a zero-sized range, which BindBufferRange
+            // a zero-sized range is INVALID_VALUE before BindBufferRange binds anything. The
-            // rejects with INVALID_VALUE before it binds anything. Floor the advertised size at
+            // shared constant is 16384 over 8 binding points, which divides.
-            // one counter per binding point.
+            *params = kFrontendMaxAtomicCounterBufferSize;
-            *params = std::max<GLint>(
-                kFrontendMaxAtomicCounterBufferSize,
-                static_cast<GLint>(GetIndexedBufferQueryPointCount(BufferTarget::AtomicCounter) * sizeof(GLuint)));
             break;
         case GL_MAX_TEXTURE_BUFFER_SIZE:
             *params = dynamicParameters.MaxTextureBufferSize;

MobileGL/MG_Test/SanityTest.cpp

@@ -17,6 +17,7 @@
 #include <MG_Backend/DirectGLES/Managers.h>
 #include <MG_Backend/DirectVulkan/BackendObject_DirectVulkan.h>
 #include <MG_Backend/BackendObjects.h>
+#include <MG_Impl/GLImpl/Buffer/GL_Buffer.h>
 #include <MG_Impl/GLImpl/Getter/GL_Getter.h>
 #include <MG_Impl/GLImpl/RenderState/GL_RenderState.h>
 #include <MG_Impl/GLImpl/Texture/GL_Texture.h>
@@ -913,6 +914,77 @@ void main() {
     MG_Backend::pActiveBackendObject.reset();
 }
 
+// KHR-GL43.shader_atomic_counters.basic-glsl-built-in, .basic-buffer-bind and .basic-api-get.
+// The atomic-counter limits used to live in two unreconciled tables - glslang compiled every
+// shader against ONE binding while glGetIntegerv advertised thirty-six - and three of the enums
+// had no case in the getter at all, so the query raised INVALID_ENUM and left the caller reading
+// whatever was in its own stack slot.
+TEST(GetterSanity, AtomicCounterQueriesMatchShaderCompilerLimits) {
+    using namespace MobileGL;
+    namespace Transpiler = MG_Util::ShaderTranspiler;
+
+    auto previousContext = Move(MG_State::pGLContext);
+    auto previousBackend = Move(MG_Backend::pActiveBackendObject);
+    MG_State::pGLContext = MakeUnique<MG_State::GLState::GLContext>();
+    MG_Backend::pActiveBackendObject = MakeUnique<DynamicParameterBackend>(MG_Backend::DynamicBackendParameters{});
+
+    GLint reported = -1;
+    MG_Impl::GLImpl::GetIntegerv(GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS, &reported);
+    EXPECT_EQ(reported, static_cast<GLint>(Transpiler::MAX_ATOMIC_COUNTER_BUFFER_BINDINGS));
+    MG_Impl::GLImpl::GetIntegerv(GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE, &reported);
+    EXPECT_EQ(reported, static_cast<GLint>(Transpiler::MAX_ATOMIC_COUNTER_BUFFER_SIZE));
+    for (const GLenum pname : {GL_MAX_COMBINED_ATOMIC_COUNTER_BUFFERS, GL_MAX_FRAGMENT_ATOMIC_COUNTER_BUFFERS,
+                               GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS}) {
+        reported = -1;
+        MG_Impl::GLImpl::GetIntegerv(pname, &reported);
+        EXPECT_EQ(reported, static_cast<GLint>(Transpiler::MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE))
+            << "pname " << pname;
+    }
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    // glBindBufferBase sets the GENERIC binding point too (GL 4.6 6.1.1), and this is the one
+    // indexed-buffer family whose non-indexed query had no case.
+    reported = -1;
+    MG_Impl::GLImpl::GetIntegerv(GL_ATOMIC_COUNTER_BUFFER_BINDING, &reported);
+    EXPECT_EQ(reported, 0);
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    GLuint buffer = 0;
+    MG_Impl::GLImpl::GenBuffers(1, &buffer);
+    MG_Impl::GLImpl::BindBuffer(GL_ATOMIC_COUNTER_BUFFER, buffer);
+    MG_Impl::GLImpl::BufferData(GL_ATOMIC_COUNTER_BUFFER, 64, nullptr, GL_STATIC_DRAW);
+    MG_Impl::GLImpl::BindBufferBase(GL_ATOMIC_COUNTER_BUFFER, 2, buffer);
+    MG_Impl::GLImpl::GetIntegerv(GL_ATOMIC_COUNTER_BUFFER_BINDING, &reported);
+    EXPECT_EQ(static_cast<GLuint>(reported), buffer);
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    // ...and the shading language has to expand the same numbers. Each array is sized by a
+    // built-in constant and indexed at its last element with a literal, so the stage only
+    // compiles when that constant is at least what glGetIntegerv just reported - which it was
+    // not while the resource table said one.
+    const String lastBinding = std::to_string(Transpiler::MAX_ATOMIC_COUNTER_BUFFER_BINDINGS - 1);
+    const String lastBuffer = std::to_string(Transpiler::MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE - 1);
+    const String source = R"(#version 430 core
+out vec4 color;
+int mgBindings[gl_MaxAtomicCounterBindings];
+int mgCombinedBuffers[gl_MaxCombinedAtomicCounterBuffers];
+int mgFragmentBuffers[gl_MaxFragmentAtomicCounterBuffers];
+layout(binding = )" + lastBinding + R"(, offset = 0) uniform atomic_uint mgCounter;
+void main() {
+    color = vec4(float(mgBindings[)" + lastBinding + R"(] + mgCombinedBuffers[)" + lastBuffer +
+                         R"(] + mgFragmentBuffers[)" + lastBuffer + R"(] + int(atomicCounterIncrement(mgCounter))));
+}
+)";
+    auto compiled = MG_Util::ShaderTranspiler::ShaderCompiler::CompileShader({
+        .shaderType = GL_FRAGMENT_SHADER,
+        .sourceStr = source,
+    });
+    EXPECT_TRUE(compiled) << (compiled ? "" : compiled.error().log);
+
+    MG_Backend::pActiveBackendObject = Move(previousBackend);
+    MG_State::pGLContext = Move(previousContext);
+}
+
 TEST(GetterSanity, ReportsKhrSubgroupDynamicParameters) {
     using namespace MobileGL;
 

MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp

@@ -93,8 +93,8 @@ namespace MobileGL {
                 Resources.maxComputeUniformComponents = 1024;
                 Resources.maxComputeTextureImageUnits = 16;
                 Resources.maxComputeImageUniforms = 8;
-                Resources.maxComputeAtomicCounters = 8;
+                Resources.maxComputeAtomicCounters = MAX_ATOMIC_COUNTERS_PER_STAGE;
-                Resources.maxComputeAtomicCounterBuffers = 1;
+                Resources.maxComputeAtomicCounterBuffers = MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE;
                 Resources.maxVaryingComponents = 60;
                 Resources.maxVertexOutputComponents = 64;
                 Resources.maxGeometryInputComponents = 64;
@@ -132,16 +132,22 @@ namespace MobileGL {
                 Resources.maxTessControlAtomicCounters = 0;
                 Resources.maxTessEvaluationAtomicCounters = 0;
                 Resources.maxGeometryAtomicCounters = 0;
-                Resources.maxFragmentAtomicCounters = 8;
+                Resources.maxFragmentAtomicCounters = MAX_ATOMIC_COUNTERS_PER_STAGE;
-                Resources.maxCombinedAtomicCounters = 8;
+                Resources.maxCombinedAtomicCounters = MAX_ATOMIC_COUNTERS_PER_STAGE;
-                Resources.maxAtomicCounterBindings = 1;
+                // Every atomic-counter limit below is the one glGetIntegerv answers; the shared
+                // constants in Types.h are what keeps the two sides from drifting apart again.
+                // gl_MaxAtomicCounterBindings and gl_MaxAtomicCounterBufferSize expand from these
+                // (Initialize.cpp), and the binding count is also the ceiling glslang checks a
+                // `layout(binding = N) uniform atomic_uint` against - it was 1, so every counter
+                // outside binding 0 failed to compile.
+                Resources.maxAtomicCounterBindings = MAX_ATOMIC_COUNTER_BUFFER_BINDINGS;
                 Resources.maxVertexAtomicCounterBuffers = 0;
                 Resources.maxTessControlAtomicCounterBuffers = 0;
                 Resources.maxTessEvaluationAtomicCounterBuffers = 0;
                 Resources.maxGeometryAtomicCounterBuffers = 0;
-                Resources.maxFragmentAtomicCounterBuffers = 1;
+                Resources.maxFragmentAtomicCounterBuffers = MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE;
-                Resources.maxCombinedAtomicCounterBuffers = 1;
+                Resources.maxCombinedAtomicCounterBuffers = MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE;
-                Resources.maxAtomicCounterBufferSize = 16384;
+                Resources.maxAtomicCounterBufferSize = MAX_ATOMIC_COUNTER_BUFFER_SIZE;
                 Resources.maxTransformFeedbackBuffers = 4;
                 Resources.maxTransformFeedbackInterleavedComponents = 64;
                 Resources.maxCullDistances = 8;

MobileGL/MG_Util/ShaderTranspiler/Types.h

@@ -14,6 +14,35 @@ namespace MobileGL {
     namespace MG_Util {
         namespace ShaderTranspiler {
             inline const char* GLOBAL_UBO_NAME = "MGL_GLOBAL_UBO";
+            // glslang's Vulkan-relaxed parse rewrites every atomic_uint into a member of a
+            // synthesized storage block named "<this>_<GL atomic-counter binding>"
+            // (ParseContextBase::growAtomicCounterBlock). That block IS the GL atomic counter
+            // buffer, and the trailing number is the only place the GL binding survives.
+            inline const char* ATOMIC_COUNTER_BLOCK_PREFIX = "gl_AtomicCounterBlock";
+
+            // 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).
+            // They used to be two unreconciled tables: BuildTBuiltInResource compiled against one
+            // binding and glGetIntegerv advertised thirty-six.
+            //
+            // The binding count is what the backends can actually serve. glslang lowers every
+            // atomic_uint onto a storage block, so one counter BUFFER costs one of the ES
+            // driver's shader-storage binding points, and DirectGLES reserves this many at the
+            // top of that range (see AtomicCounterEsslBinding in the DirectGLES managers).
+            inline constexpr Int MAX_ATOMIC_COUNTER_BUFFER_BINDINGS = 8;
+            // GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE, in basic machine units. Independent of the
+            // counter COUNTS below - it bounds the byte offset a counter may be declared at, and
+            // the conformance suite declares counters well past the eighth one (offsets 32 and
+            // 128 in a two-counter buffer). KHR-GL44.multi_bind splits it evenly across every
+            // advertised binding point and binds them all in one glBindBuffersRange, so it must
+            // stay a multiple of, and comfortably larger than, four times the binding count.
+            inline constexpr Int MAX_ATOMIC_COUNTER_BUFFER_SIZE = 16384;
+            // GL_MAX_{FRAGMENT,COMPUTE,COMBINED}_ATOMIC_COUNTER_BUFFERS and the matching
+            // _ATOMIC_COUNTERS. Eight is the GL 4.6 core minimum for the compute stage
+            // (table 23.45) and every other stage this implementation serves counters on.
+            inline constexpr Int MAX_ATOMIC_COUNTER_BUFFERS_PER_STAGE = 8;
+            inline constexpr Int MAX_ATOMIC_COUNTERS_PER_STAGE = 8;
 
             struct EmptyType {};