From 0acfa9aab9da8f5a1cd11c5e43bdc9663a2e93d4 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 22 Sep 2026 15:53:27 -0400 Subject: [PATCH] [MG_State, MG_Backend, MG_Test, docs] (P7): the reflection archive publishes the storage-block index space, and both magma consumers read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CONTRACT-P7 §5.3, OQ-8. MagmaProgramSource::EnsureStorageBlocks re-ran SPIRV-Reflect over every stage module of the program PER DRAW: the class is a borrowed view that WireDraw.inc's WireProgramSource constructs afresh for each wire draw, so its `mutable` memo was cold every time it was read. The list it rebuilt is a pure function of the linked program, so it is now built once at link time and travels in the archive. LinkArtifacts gains storageBlocks{name, binding, dataSize} with its own VisitFields table, the codec version goes 2 -> 3, and the schema fingerprint and wireFingerprint move with it - which is expected, and is what makes a mixed-version pair refuse at the handshake instead of at the first program. FILLED IN PHASE A, and that is not where it looks like it belongs. The only function that sees both finished halves is ProgramSpirvTask's L1 cache insert, but its writes reach the TRANSLATION CACHE only: phase A's artifacts have already been moved into the ProgramObject by then. A field filled there would be present on a cache hit and absent on every cold link. So the fill is glslang-derived, and the equivalence is MEASURED rather than assumed: ProgramTest.TheArchivesStorageBlockOrderIsTheOneSpirvReflectProduces runs the old consumer's own algorithm over the real modules and compares element for element, on a multi-stage program with an arrayed SSBO and an atomic-counter block - the three cases that could have diverged (the instance array collapses to one entry, the synthesized counter block IS in the backend's space although GL's enumeration excludes it, and the cross-stage dedupe walks stages in linked order). MEASURED, same binary and same window (frames=1 draws=1 acc=18, identical bytes), DirectVulkan x inproc over the SSBO scenarios: spvReflectCreateShaderModule on the wire draw path 4 -> 0. The 0 is structural: the call site is gone and MagmaProgramSource.h no longer includes spirv_reflect.h at all, which is the gate a future edit breaks on purpose. THE MONOLITH CONSUMER IS SWITCHED TOO, under #if symmetry, so the disaggregated build retires g_programResourceCaches, ProgramResourceCache, StorageBlockResource, BufferVariableResource, AddBufferVariablesRecursive, NormalizeDescriptorName, GetProgramResourceCache and the rehash hazard - the open-addressed-map dangling reference that was a reproducible segfault in ProgramPipelineScenario's two storage-block cases. Nothing outside DirectVulkan.cpp read any of it (measured; GL_BUFFER_VARIABLE and GL_ACTIVE_VARIABLES are answered by ProgramInterface.cpp out of a different index space), so the two arms differ in exactly the two functions their callers use. THE PULL BUILD KEEPS ALL OF IT, and the integrator should see why: the archive member is MOBILEGL_BUILD_DISAGGREGATED-only because one more Vector in LinkArtifacts changes its size, its implicit destructor and its move constructor, and G1 pins the pull build's .text byte for byte. So §5.3's deletion is achieved in the disaggregated build and not in the pull one; retiring the pull half is a one-line change the day that pin is retired or the member moves. Filed as P13 in notes/p7/magma-c.md. One improvement rides along: the monolith binding getter now asks the program for a rebind on every read, over an immutable list, where it used to read a binding patched into a mutable cache - so a rebind can no longer be lost to a cache rebuild that raced a state-version bump. G1: .text 0xa52203 unchanged, 27837 -> 27837 defined symbols, 0 added / 0 removed / 0 resized / 0 renamed, file byte-identical. An unguarded call site caught the guard rule once mid-slice by failing the pull compile, which is the loud version. RED-ONCE: publishing nothing reds the order-equivalence case (with the reference list printed) and 25 of 60 storage-block scenarios - the MONOLITH DirectVulkan arm beside all three transports, which is the evidence that the monolith consumer really was switched. Restored: 61/61. --- .../MG_Backend/DirectVulkan/DirectVulkan.cpp | 104 +++++++++++ .../Renderer/MagmaProgramSource.h | 96 +++++----- .../GLState/ProgramState/ProgramArtifacts.h | 73 +++++++- .../ProgramState/ProgramArtifactsCodec.h | 9 +- .../GLState/ProgramState/ProgramLinkTask.cpp | 111 ++++++++++++ .../GLState/ProgramState/ProgramLinkTask.h | 8 + .../GLState/ProgramState/ProgramObject.h | 4 + .../Program/ProgramArtifactsCodecTest.cpp | 15 +- .../MG_Test/Program/ProgramArtifactsTest.cpp | 8 + MobileGL/MG_Test/Program/ProgramTest.cpp | 171 ++++++++++++++++++ docs/Disaggregated/notes/p7/magma-c.md | 121 +++++++++++++ 11 files changed, 663 insertions(+), 57 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp index c1979280..1f439970 100644 --- a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp @@ -131,6 +131,21 @@ namespace MobileGL::MG_Backend::DirectVulkan { } namespace { +#if !MOBILEGL_BUILD_DISAGGREGATED + // P7 wave 2 package C, OQ-8: THE PULL BUILD'S HALF, VERBATIM. A disaggregated build + // answers GetShaderStorageBlock{Index,Binding} out of LinkArtifacts::storageBlocks + // instead (see those functions below), which retires this whole cache - the map, the + // SPIRV-Reflect rebuild, the teardown clear and the rehash hazard. It cannot be + // retired HERE because the archive member it reads is itself disaggregated-only, for + // G1: one more Vector in LinkArtifacts moves the pull build's .text, which is pinned + // byte-for-byte at 0xa52203. + // + // NOTHING OUTSIDE THIS FILE READS ANY OF IT (measured: BufferVariableResource, + // bufferVariables, activeVariables and StorageBlockResource have no reference anywhere + // else in the tree, and `dataSize` is written and never read - GL_BUFFER_VARIABLE and + // GL_ACTIVE_VARIABLES are answered by MG_Impl/GLImpl/Program/ProgramInterface.cpp out + // of glslang's own reflection, which is a different index space). So the two arms + // below differ in exactly the two functions their callers use, and in nothing else. struct BufferVariableResource { String name; GLuint blockIndex = 0; @@ -162,6 +177,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { Vector storageBlocks; Vector bufferVariables; }; +#endif // !MOBILEGL_BUILD_DISAGGREGATED struct DrawElementsIndirectCommand { Uint32 count = 0; @@ -178,12 +194,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint32 baseInstance = 0; }; +#if !MOBILEGL_BUILD_DISAGGREGATED // Keyed by GL program name so the freed-name reuse in IndexGenerator bounds the // map at the peak-simultaneous-program high-water mark; each slot's ownership is // checked against the program's lifetime id before it is served (see // GetProgramResourceCache). Cleared wholesale at EGL teardown via // ClearProgramResourceCaches. UnorderedMap g_programResourceCaches; +#endif void ClearReadPixelsOutput(GLsizei width, GLsizei height, GLenum format, GLenum type, void* pixels) { if (!pixels || width <= 0 || height <= 0) { @@ -198,6 +216,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } +#if !MOBILEGL_BUILD_DISAGGREGATED String NormalizeDescriptorName(const SpvReflectDescriptorBinding& binding) { const char* rawName = binding.name; if (binding.type_description != nullptr && binding.type_description->type_name != nullptr) { @@ -348,6 +367,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } return cache; } +#endif // !MOBILEGL_BUILD_DISAGGREGATED #if MOBILEGL_BUILD_DISAGGREGATED // The verb's handles identify server stores. Readback orders GPU-produced @@ -448,6 +468,67 @@ namespace MobileGL::MG_Backend::DirectVulkan { } // namespace +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 wave 2 package C, OQ-8 (CONTRACT-P7 §5.3): THE MONOLITH CONSUMER READS THE ARCHIVE + // TOO, so one published list serves both of this backend's arms. + // + // THE TWO ARMS BELOW ARE NOT TWO ANSWERS. ProgramTest's + // TheArchivesStorageBlockOrderIsTheOneSpirvReflectProduces runs the OLD arm's algorithm - + // SPIRV-Reflect over the real modules, sorted and deduplicated exactly as + // GetProgramResourceCache does it - over a multi-stage program with an arrayed SSBO and an + // atomic-counter block, and asserts element-for-element equality with what the archive + // published. That equivalence is what makes an #if here a build-time SELECTION rather than + // a behavioural fork, and it is the reason the case exists. + // + // WHY AN #if AT ALL, AND WHAT IT COSTS. LinkArtifacts::storageBlocks is + // MOBILEGL_BUILD_DISAGGREGATED-only, because one more Vector member changes the struct's + // size, its implicit destructor and its move constructor in the PULL build whose .text G1 + // pins byte-for-byte (0xa52203). So the pull build keeps `g_programResourceCaches`, the + // SPIRV-Reflect rebuild and `ClearProgramResourceCaches`, verbatim; only this build drops + // them - and with them the rehash hazard the ordering comment below the block-binding + // setter documents. Retiring the pull half is the day the member stops needing its guard, + // which is a G1/P13 decision and not a wave-2 one; recorded in notes/p7/magma-c.md. + void ClearProgramResourceCaches() { + // Nothing to clear: there is no cache in this build. Kept as an entry point so + // BackendObject_DirectVulkan's two EGL-teardown call sites stay statement for + // statement what they are in the pull build. + } + + GLuint GetShaderStorageBlockIndex(const MG_State::GLState::ProgramObject& program, const String& name) { + const auto& blocks = program.GetLinkReflection().storageBlocks; + const auto find = [&blocks](const String& key) { + return std::find_if(blocks.begin(), blocks.end(), + [&](const MG_State::GLState::StorageBlockReflection& block) { return block.name == key; }); + }; + auto it = find(name); + if (it == blocks.end()) { + // Archive names are normalised, so an arrayed block that GL enumerates per element + // - "B[0]", "B[1]" - is one entry here, spelled "B". Same second chance the cache + // gave, for the same callers. + const auto bracket = name.rfind('['); + if (bracket == String::npos || name.empty() || name.back() != ']') return GL_INVALID_INDEX; + it = find(name.substr(0, bracket)); + if (it == blocks.end()) return GL_INVALID_INDEX; + } + return static_cast(std::distance(blocks.begin(), it)); + } + + GLuint GetShaderStorageBlockBinding(const MG_State::GLState::ProgramObject& program, GLuint blockIndex) { + const auto& blocks = program.GetLinkReflection().storageBlocks; + if (blockIndex >= blocks.size()) { + return 0; + } + // THE OVERRIDE IS APPLIED ON READ, where the cache used to apply it on rebuild and + // patch it in place on rebind. Same answer by a shorter route: the program is the + // authoritative record of a rebound block (it is what GL_BUFFER_BINDING reports), the + // archive carries the DECLARED binding only, and asking the program here means a + // rebind can no longer be lost to a cache rebuild that happened to race a state-version + // bump. + const Int rebound = program.GetShaderStorageBlockBindingOverride(blocks[blockIndex].name); + if (rebound >= 0) return static_cast(rebound); + return blocks[blockIndex].binding; + } +#else void ClearProgramResourceCaches() { // Called from EGL teardown while the backend's m_eglStateMutex is held; GL // calls are serialized in this codebase (contexts migrate threads but never @@ -483,6 +564,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } return cache.storageBlocks[blockIndex].binding; } +#endif void ClearBufferfi(GLenum buffer, GLint drawbuffer, GLfloat depth, GLint stencil) { MOBILEGL_ASSERT(pVulkanRenderer, "DirectVulkan::ClearBufferfi called with null VulkanRenderer"); @@ -949,6 +1031,27 @@ namespace MobileGL::MG_Backend::DirectVulkan { MakeUnique("DirectVulkan", __func__, "Shader storage binding is out of range.")); return; } +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 wave 2 package C, OQ-8: NOTHING LEFT TO PATCH, and the hazard goes with it. + // + // The frontend has already recorded the new binding on the program, and that record is + // now what GetShaderStorageBlockBinding reads (it asks + // GetShaderStorageBlockBindingOverride on every call, over an IMMUTABLE archive list). + // What stood here was the other half of a mutable cache: resolve the index, take a + // reference into g_programResourceCaches and patch `binding` in place so an + // already-built entry need not be thrown away. + // + // THE HAZARD THAT CAME WITH IT IS GONE TOO, and it is worth naming because it was a + // real crash rather than a theoretical one: GetShaderStorageBlockIndex re-entered + // GetProgramResourceCache, which indexes an OPEN-ADDRESSED map and can therefore + // insert, and a rehash moves entries - so a reference taken before that call dangled. + // Binding one program's storage block while another program's entry was still absent + // from the cache was a reproducible segfault (ProgramPipelineScenario's two + // storage-block cases, in one process). The ordering above was the fix; having no + // cache is the retirement. The pull build below keeps both. + (void)storageBlockBinding; + } +#else // The frontend already validated that the name denotes an active block, and has // already recorded the new binding on the program - which is what reseeds this cache // whenever it is rebuilt. Writing the entry here as well keeps an ALREADY-BUILT cache @@ -967,6 +1070,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (blockIndex >= cache.storageBlocks.size()) return; cache.storageBlocks[blockIndex].binding = storageBlockBinding; } +#endif void ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, void* pixels) { MOBILEGL_ASSERT(pVulkanRenderer, "DirectVulkan::ReadPixels called with null VulkanRenderer"); MOBILEGL_ASSERT(MGB_CTX_LIVE, "DirectVulkan::ReadPixels called with null GL context"); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaProgramSource.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaProgramSource.h index 9b671cbd..e1ba417f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaProgramSource.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaProgramSource.h @@ -7,7 +7,11 @@ #include #include #include "../DirectVulkanResourceState.h" -#include +// P7 wave 2 package C, OQ-8: spirv_reflect.h is GONE from this header, and its absence is the +// gate. Everything this class needed it for - the storage-block index space - now travels in +// the archive (LinkArtifacts::storageBlocks), so the SPIR-V reflector is off the Magma wire +// draw path entirely. A future edit that re-adds the include is re-adding a per-draw reflect, +// because a MagmaProgramSource is constructed once per draw and can memoise nothing. #endif namespace MobileGL::MG_Backend::DirectVulkan { @@ -153,30 +157,49 @@ namespace MobileGL::MG_Backend::DirectVulkan { GLenum GetTransformFeedbackBufferMode() const { return IsWire() ? Link().xfbBufferMode : m_frontend->GetTransformFeedbackBufferMode(); } + // P7 wave 2 package C, OQ-8 (CONTRACT-P7 §5.3): READ OUT OF THE ARCHIVE. + // + // What stood here re-ran spvReflectCreateShaderModule over EVERY stage module of the + // program, and it did so PER DRAW: a MagmaProgramSource is a borrowed view that + // WireDraw.inc's WireProgramSource constructs afresh for each wire draw, so the lazy + // `m_storageBlocksReady` latch could never outlive one. The list it rebuilt is a pure + // function of the linked program, so it is now built once at link time and travels in + // the archive (LinkArtifacts::storageBlocks), which takes the SPIR-V reflector off the + // wire draw path entirely - this header no longer includes spirv_reflect.h at all. Uint GetShaderStorageBlockIndex(const String& name) const { if (!IsWire()) return DirectVulkan::GetShaderStorageBlockIndex(*m_frontend, name); - EnsureStorageBlocks(); - String key = name; - auto find = [&] { - return std::find_if(m_storageBlocks.begin(), m_storageBlocks.end(), - [&](const StorageBlock& block) { return block.Name == key; }); + const auto& blocks = Link().storageBlocks; + // The archive carries NORMALISED names (the array subscript stripped), because + // that is the spelling SPIRV-Reflect's type_name gives and the spelling + // ProgramFactory::ReflectLayout looks up. A caller that still holds a GL-style + // "Foo[2]" gets the same second chance it always had. + const auto find = [&blocks](const String& key) { + return std::find_if(blocks.begin(), blocks.end(), + [&](const MG_State::GLState::StorageBlockReflection& block) { + return block.name == key; + }); }; - auto found = find(); - if (found == m_storageBlocks.end() && !name.empty() && name.back() == ']') { + auto found = find(name); + if (found == blocks.end() && !name.empty() && name.back() == ']') { const auto bracket = name.rfind('['); - if (bracket != String::npos) { key = name.substr(0, bracket); found = find(); } + if (bracket != String::npos) found = find(name.substr(0, bracket)); } - return found == m_storageBlocks.end() ? GL_INVALID_INDEX : static_cast(found - m_storageBlocks.begin()); + return found == blocks.end() ? GL_INVALID_INDEX + : static_cast(found - blocks.begin()); } Uint GetShaderStorageBlockBinding(Uint index) const { if (!IsWire()) return DirectVulkan::GetShaderStorageBlockBinding(*m_frontend, index); - EnsureStorageBlocks(); - if (index >= m_storageBlocks.size()) return 0; - const auto& block = m_storageBlocks[index]; + const auto& blocks = Link().storageBlocks; + if (index >= blocks.size()) return 0; + const auto& block = blocks[index]; + // THE OVERRIDE STILL WINS, and it still comes off the RECORD rather than the + // archive: glShaderStorageBlockBinding is a post-link mutable and the archive + // carries link-time defaults only (rule on MagmaProgramSource, and the reason + // StorageBlockReflection::binding is documented as the DECLARED binding). for (const auto& override : m_record->StorageOverrides) { - if (override.Name == block.Name) return override.Binding; + if (override.Name == block.name) return override.Binding; } - return block.Binding; + return block.binding; } private: @@ -187,48 +210,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { return tIndex >= 0 && static_cast(tIndex) < Link().blockReflection.size() ? &Link().blockReflection[tIndex] : nullptr; } - struct StorageBlock { String Name; Uint Binding; }; - void EnsureStorageBlocks() const { - if (m_storageBlocksReady) return; - m_storageBlocksReady = true; - // Match the legacy resource-query index order: stage order, then - // normalized descriptor name/binding, deduplicated across stages. - for (const auto& spirv : GetGeneratedSpirv()) { - if (spirv.empty()) continue; - SpvReflectShaderModule module{}; - if (spvReflectCreateShaderModule(spirv.size() * sizeof(Uint), spirv.data(), &module) != SPV_REFLECT_RESULT_SUCCESS) continue; - Uint32 count = 0; - if (spvReflectEnumerateDescriptorBindings(&module, &count, nullptr) == SPV_REFLECT_RESULT_SUCCESS) { - Vector bindings(count); - if (spvReflectEnumerateDescriptorBindings(&module, &count, bindings.data()) == SPV_REFLECT_RESULT_SUCCESS) { - auto name = [](const SpvReflectDescriptorBinding* binding) { - const char* raw = binding->name; - if (binding->type_description && binding->type_description->type_name) raw = binding->type_description->type_name; - String result = raw ? raw : ""; - const auto suffix = result.find("[0]"); - return suffix == String::npos ? result : result.substr(0, suffix); - }; - std::sort(bindings.begin(), bindings.end(), [&](const auto* a, const auto* b) { - const String an = a ? name(a) : String(), bn = b ? name(b) : String(); - return an != bn ? an < bn : (a && b && a->binding < b->binding); - }); - for (const auto* binding : bindings) { - if (!binding || binding->descriptor_type != SPV_REFLECT_DESCRIPTOR_TYPE_STORAGE_BUFFER) continue; - const String key = name(binding); - if (key.empty() || std::any_of(m_storageBlocks.begin(), m_storageBlocks.end(), - [&](const StorageBlock& block) { return block.Name == key; })) continue; - m_storageBlocks.push_back({key, binding->binding}); - } - } - } - spvReflectDestroyShaderModule(&module); - } - } const Program* m_frontend = nullptr; MG_Pipe::MGPipeHandle m_handle = MG_Pipe::kMGPipeNullHandle; const MG_Pipe::MGPipeShaderCsoRecord* m_record = nullptr; - mutable Bool m_storageBlocksReady = false; - mutable Vector m_storageBlocks; + // P7 OQ-8: the two mutables that memoised the per-draw reflect are gone with it. They + // were `mutable` precisely because this class is a borrowed const view - and that is + // also why they never helped: a view lives for one draw, so the latch was cold every + // time it was read. }; #else using MagmaProgramSource = MG_State::GLState::ProgramObject; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h index 6aa681e0..4126f3ff 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h @@ -90,6 +90,47 @@ namespace MobileGL::MG_State::GLState { using PipeInputReflection = ResourceReflection; using PipeOutputReflection = ResourceReflection; +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 wave 2 package C, OQ-8 (CONTRACT-P7 §5.3): THE SHADER STORAGE BLOCKS AS DIRECTVULKAN + // INDEXES THEM, published once at link time instead of re-derived per draw. + // + // WHY A SECOND VIEW OF SOMETHING blockReflection ALREADY HOLDS. `blockReflection` is + // glslang's list, in TProgram index order, carrying uniform blocks, storage blocks and the + // synthesized atomic-counter blocks together, one entry per ARRAY ELEMENT. DirectVulkan's + // resource queries speak a different index space entirely - the one its SPIRV-Reflect walk + // produces: storage descriptors only, normalised block name with the array subscript + // stripped, deduplicated across stages. `GetShaderStorageBlockIndex(name)` hands that index + // out and `GetShaderStorageBlockBinding(index)` reads it back, so the two have to agree + // with each other and with the names ProgramFactory::ReflectLayout looks up (which come + // from its OWN reflect pass over the same modules). This vector IS that space, built once. + // + // WHAT IT COSTS AND WHAT IT BUYS. MagmaProgramSource is a BORROWED VIEW constructed afresh + // for every wire draw (WireDraw.inc's WireProgramSource), so its lazy + // `EnsureStorageBlocks` re-ran spvReflectCreateShaderModule over every stage module of + // every program on every draw that touched an SSBO. Reading it out of the archive instead + // takes the SPIR-V reflector off the wire draw path entirely. + // + // `binding` IS THE DECLARED (or IO-mapper-assigned) BINDING AND NOT THE CURRENT ONE. + // glShaderStorageBlockBinding travels separately - as the record's StorageOverrides on the + // wire and as ProgramObject::GetShaderStorageBlockBindingOverride in the monolith - and + // both consumers apply it on read, exactly as they did when this list was rebuilt per + // draw. Baking the current binding in would freeze a rebind at link time. + // + // DISAGGREGATED-ONLY, and that guard is load-bearing for G1: LinkArtifacts is a pull-build + // type too, and one more Vector member would change its size, its implicit destructor and + // its move constructor in a build whose .text must stay byte-identical. + struct StorageBlockReflection { + // The NORMALISED name: the SPIR-V block type name with a trailing array subscript + // stripped, which is what SpvReflectDescriptorBinding's type_name answers and what + // ProgramFactory::ReflectLayout looks blocks up by. + String name; + Uint32 binding = 0; + // GL_BUFFER_DATA_SIZE. Carried because the monolith resource-query consumer reports + // it; the wire consumer does not read it today. + Int32 dataSize = 0; + }; +#endif + // Transform feedback (GL 3.0 core: glTransformFeedbackVaryings applies on // the NEXT link; the linked snapshot below is what draws and queries see). struct XfbVarying { @@ -167,6 +208,12 @@ namespace MobileGL::MG_State::GLState { // The owned reflection snapshot. Indexed by TProgram index; see the structs above. Vector uniformReflection; Vector blockReflection; +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8: DirectVulkan's storage-block index space, filled beside blockReflection in + // SnapshotGlslangReflection. See StorageBlockReflection above for what the order is + // and why it is not blockReflection's. + Vector storageBlocks; +#endif Vector pipeInputReflection; Vector pipeOutputReflection; // Program-level scalars glslang answers off the linked intermediates. @@ -479,14 +526,32 @@ namespace MobileGL::MG_State::GLState { v("blockMemberElement", a.blockMemberElement); } // 11 fields +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8. Three plain fields, so the generic codec arms carry it with no new arm of their + // own - which is the whole reason it is a table rather than hand-written bytes. + template + requires std::same_as, StorageBlockReflection> + void VisitFields(Self& a, V&& v) { + v("name", a.name); + v("binding", a.binding); + v("dataSize", a.dataSize); + } // 3 fields +#endif + // Every member EXCEPT `program`: it is null for every archived instance by construction // (ProgramTranslationCache.h asserts that at insert) and must never be serialized - it is - // the live glslang TProgram that only DoReflection may touch. 57 of the 58 members. + // the live glslang TProgram that only DoReflection may touch. 57 of the 58 members in a + // pull/monolith build, 58 of 59 in a disaggregated one (P7 OQ-8's storageBlocks). template requires std::same_as, LinkArtifacts> void VisitFields(Self& a, V&& v) { v("uniformReflection", a.uniformReflection); v("blockReflection", a.blockReflection); +#if MOBILEGL_BUILD_DISAGGREGATED + // Beside blockReflection because that is where it is FILLED, and a visitor that walks + // the table in declaration order then reads the two together. + v("storageBlocks", a.storageBlocks); +#endif v("pipeInputReflection", a.pipeInputReflection); v("pipeOutputReflection", a.pipeOutputReflection); v("lastStageIsFragment", a.lastStageIsFragment); @@ -569,7 +634,13 @@ namespace MobileGL::MG_State::GLState { #if defined(__GLIBCXX__) && !defined(_GLIBCXX_DEBUG) && (SIZE_MAX == UINT64_MAX) #define MGL_RESOURCEREFLECTION_SIZE 128 #define MGL_XFBVARYING_SIZE 128 +#if MOBILEGL_BUILD_DISAGGREGATED +// P7 OQ-8 added storageBlocks, one more Vector, in the disaggregated build ONLY. The pull +// number below is the one G1 measures and it has not moved. +#define MGL_LINKARTIFACTS_SIZE 1080 +#else #define MGL_LINKARTIFACTS_SIZE 1056 +#endif #define MGL_SPIRVARTIFACTS_SIZE 88 #elif defined(_LIBCPP_VERSION) && (SIZE_MAX == UINT64_MAX) && defined(MGL_ARTIFACT_SIZES_LIBCXX_PINNED) // The integrator pins these from the NDK build (brief C.4); until then this branch is inert. diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.h b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.h index 3391cc45..5fd7fe95 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.h @@ -61,7 +61,14 @@ namespace MobileGL::MG_State::GLState { // Bumped whenever the byte format changes in a way a previous reader would misread. A // reader that sees a different word REFUSES; it never tries to guess a layout. #if MOBILEGL_BUILD_DISAGGREGATED - inline constexpr Uint32 kProgramArtifactsCodecVersion = 2; + // 3 since P7 wave 2 package C (OQ-8): LinkArtifacts gained `storageBlocks`, so a v2 reader + // would run out of bytes in the middle of the stream rather than notice. The schema + // fingerprint beside the version would catch it on its own - it is derived from the + // VisitFields tables and therefore moved with the new row - but the version is what a + // reader checks FIRST and what the refusal names, and CONTRACT-P7 §5.3 asks for the bump + // explicitly. `wireFingerprint` moves with both, which is expected and is what makes a + // mixed-version pair refuse at the handshake instead of at the first program. + inline constexpr Uint32 kProgramArtifactsCodecVersion = 3; // Derived from the actual VisitFields order/names, container element schemas and scalar // representations. No native container size/offset is included. Also checked in Hello. Uint64 ProgramArtifactsSchemaFingerprint(); diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 5522ccba..5afe23f3 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -106,6 +106,29 @@ namespace { return bracket == MobileGL::String::npos ? name : name.substr(0, bracket); } +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8. ShaderStage -> EShLanguage, so a reflection record's `stages` mask (which is an + // EShLanguageMask) can be tested against the stage of a snapshotted shader. Written here + // rather than lifted into a shared header because this is the only caller and + // ProgramArtifacts.h's include-closure probe forbids this file's neighbours from growing a + // dependency on ShaderStage.h. EShLangCount means "no glslang stage", which the caller + // skips rather than guesses at. + static EShLanguage ShaderStageToEShLanguage(MobileGL::ShaderStage stage) { + switch (stage) { + case MobileGL::ShaderStage::Vertex: return EShLangVertex; + case MobileGL::ShaderStage::TessControl: return EShLangTessControl; + case MobileGL::ShaderStage::TessEval: return EShLangTessEvaluation; + case MobileGL::ShaderStage::Geometry: return EShLangGeometry; + case MobileGL::ShaderStage::Fragment: return EShLangFragment; + case MobileGL::ShaderStage::Compute: return EShLangCompute; + case MobileGL::ShaderStage::ShaderStageCount: + case MobileGL::ShaderStage::Unknown: + break; + } + return EShLangCount; + } +#endif + // Element index of an arrayed interface-block instance: "GOKU[3]" -> 3, "GOKU" -> 0. // Reflection spells arrayed instances exactly this way (glslang expands the instance // array into one TObjectReflection per element), and the subscript it writes is a plain @@ -1735,9 +1758,97 @@ namespace MobileGL::MG_State::GLState { } SnapshotGlslangReflection(); +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8. Guarded at the CALL as well as at the definition, which is not belt and + // braces: LinkArtifacts::storageBlocks does not exist in a pull build, so neither can + // the function that fills it, and G1 requires this translation unit to be statement + // for statement what it was there. + SnapshotStorageBlockIndexSpace(); +#endif return true; } +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 wave 2 package C, OQ-8 (CONTRACT-P7 §5.3): DIRECTVULKAN'S STORAGE-BLOCK INDEX SPACE, + // BUILT ONCE HERE INSTEAD OF PER DRAW. + // + // WHAT ORDER THIS HAS TO PRODUCE, and it is not blockReflection's. The consumers speak the + // index space DirectVulkan's SPIRV-Reflect walk produces (DirectVulkan.cpp's + // GetProgramResourceCache, and the copy of it MagmaProgramSource used to carry): + // + // for each SPIR-V module, IN LINKED-STAGE ORDER + // take its STORAGE_BUFFER descriptors + // sorted by (normalised block name, binding) + // appending any name not already present + // + // Every clause is reproducible from what phase A already owns. `in.shaders` is the linked + // shader snapshot and is documented as ALREADY STAGE-SORTED, and it is the same list, in + // the same order, that ProgramSpirvTask turns into `generatedSpirv` one module per entry - + // so iterating it iterates the modules. `blockReflection[].stages` is an EShLanguageMask, + // which is what says whether a given block is one of a given module's descriptors. The + // name normalisation is StripArrayElementSuffix, which is the same "[k]" strip + // NormalizeDescriptorName does to the SPIR-V type name: glslang expands an INSTANCE ARRAY + // into one reflection entry per element ("Foo[0]", "Foo[1]") while SPIR-V carries one + // descriptor with a count, so stripping is what makes the two agree on ONE entry named + // "Foo". + // + // ATOMIC-COUNTER BLOCKS ARE INCLUDED, deliberately, and they are the case that makes the + // filter `type.isBuffer` rather than IsStorageBlock(). The transpiler lowers every + // atomic_uint onto a synthesized gl_AtomicCounterBlock_ BUFFER block, so + // SPIRV-Reflect sees an ordinary STORAGE_BUFFER descriptor for it and DirectVulkan's index + // space has always contained it. GL's own GL_SHADER_STORAGE_BLOCK enumeration excludes + // them (ProgramInterface.cpp's ClassifyBlock does the exclusion there, in the space that + // needs it) - these are two different index spaces and this is the backend's. + // + // `binding` IS THE DECLARED ONE. A rebind travels separately and both consumers apply it + // on read; see StorageBlockReflection in ProgramArtifacts.h. + void ProgramLinkTask::SnapshotStorageBlockIndexSpace() { + artifacts.storageBlocks.clear(); + const auto alreadyPresent = [this](const String& name) { + for (const auto& block : artifacts.storageBlocks) { + if (block.name == name) return true; + } + return false; + }; + for (const LinkShaderInput& shader : in.shaders) { + const EShLanguage stage = ShaderStageToEShLanguage(shader.stage); + if (stage == EShLangCount) continue; + const Uint32 stageBit = 1u << static_cast(stage); + + // Pointers, not copies: blockReflection is stable for the rest of this function and + // a BlockReflection carries several strings. + Vector ofThisStage; + for (const ProgramObject::BlockReflection& block : artifacts.blockReflection) { + if (!block.type.isBuffer) continue; + if ((block.stages & stageBit) == 0) continue; + ofThisStage.push_back(&block); + } + // STABLE, so two entries that normalise to one name keep their declaration order + // and the FIRST (element 0, the base binding) is the one that survives the dedupe - + // which is the element SPIRV-Reflect's single descriptor reports. + std::stable_sort(ofThisStage.begin(), ofThisStage.end(), + [](const ProgramObject::BlockReflection* lhs, + const ProgramObject::BlockReflection* rhs) { + const String lhsName = StripArrayElementSuffix(lhs->name); + const String rhsName = StripArrayElementSuffix(rhs->name); + if (lhsName != rhsName) return lhsName < rhsName; + return lhs->binding < rhs->binding; + }); + for (const ProgramObject::BlockReflection* block : ofThisStage) { + const String name = StripArrayElementSuffix(block->name); + if (name.empty() || alreadyPresent(name)) continue; + ProgramObject::StorageBlockReflection record; + record.name = name; + record.binding = block->binding < 0 ? 0u : static_cast(block->binding); + record.dataSize = static_cast(block->size); + artifacts.storageBlocks.push_back(Move(record)); + } + } + MGLOG_D("ProgramObject %u: Reflection - %zu storage block(s) in the backend index space", + in.externalIndex, artifacts.storageBlocks.size()); + } +#endif + // The last thing DoReflection does, and the thing that lets everything after it stop // caring that a glslang::TProgram ever existed: copy every reflection record the GL query // surface reads into LinkArtifacts' own owned tables. diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h index 245f2665..07dc26e3 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h @@ -207,6 +207,14 @@ namespace MobileGL::MG_State::GLState { // one and overwritten the qualifier. See the definition for why the invented binding is // deliberately left in place for the backends' own use. void SeedDefaultStorageBlockBindings(); +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 wave 2 package C, OQ-8: builds LinkArtifacts::storageBlocks - DirectVulkan's own + // storage-block index space - out of the reflection snapshot the call above produced, + // so the backends stop re-deriving it with SPIRV-Reflect on every draw. Runs after + // SnapshotGlslangReflection because it reads blockReflection. See the definition for + // which order it has to reproduce and why each clause of it is derivable here. + void SnapshotStorageBlockIndexSpace(); +#endif Bool ValidateFragmentOutputLocations(); Bool ResolveTransformFeedbackVaryings(); void ResolveGsTriangleStripCapture(const glslang::TIntermediate* captureIntermediate); diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index 7546e5aa..33a4de73 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -48,6 +48,10 @@ namespace MobileGL::MG_State::GLState { using BlockReflection = ResourceReflection; using PipeInputReflection = ResourceReflection; using PipeOutputReflection = ResourceReflection; +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8: DirectVulkan's storage-block index space, carried in LinkArtifacts. + using StorageBlockReflection = MobileGL::MG_State::GLState::StorageBlockReflection; +#endif using XfbVarying = MobileGL::MG_State::GLState::XfbVarying; using LinkArtifacts = MobileGL::MG_State::GLState::LinkArtifacts; using SpirvArtifacts = MobileGL::MG_State::GLState::SpirvArtifacts; diff --git a/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp b/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp index a5f71488..3290e206 100644 --- a/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp @@ -362,7 +362,15 @@ TEST(ProgramArtifactsCodec, AVersionMismatchIsRefused) { // no archive owns. A codec arm for it would be a use-after-free waiting for a cache hit. TEST(ProgramArtifactsCodec, TheTablesVisitEveryMemberExceptTheLiveProgram) { #if MOBILEGL_PIPE_PUSH +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8: 59 members, 58 visited. The 59th is still the live TProgram; the 58th is + // storageBlocks, which exists only in this build (ProgramArtifacts.h guards it so the pull + // build's LinkArtifacts keeps its size, its destructor and its .text). + EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 58u); + EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 3u); +#else EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 57u); +#endif EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 8u); EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 14u); EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 11u); @@ -381,7 +389,12 @@ TEST(ProgramArtifactsCodec, PortableHeaderUsesWireSchemaRatherThanNativeContaine Uint64 schema = 0; std::memcpy(&version, bytes.data(), sizeof(version)); std::memcpy(&schema, bytes.data() + sizeof(version), sizeof(schema)); - EXPECT_EQ(version, 2u); + // 3 since P7 OQ-8 (CONTRACT-P7 §5.3): LinkArtifacts gained storageBlocks, so a v2 reader + // would run out of bytes mid-stream rather than notice. The schema word beside it moved + // too - it is derived from the VisitFields tables - and so did `wireFingerprint`, which is + // what makes a mixed-version pair refuse at the handshake instead of at the first program. + EXPECT_EQ(version, kProgramArtifactsCodecVersion); + EXPECT_EQ(version, 3u); EXPECT_EQ(schema, ProgramArtifactsSchemaFingerprint()); EXPECT_NE(schema, 0u); EXPECT_NE(schema, sizeof(LinkArtifacts)); diff --git a/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp b/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp index e1f9239b..9cf7555b 100644 --- a/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp @@ -81,7 +81,15 @@ namespace { EXPECT_EQ(CountFields(), 20u); EXPECT_EQ(CountFields(), 14u); EXPECT_EQ(CountFields(), 11u); +#if MOBILEGL_BUILD_DISAGGREGATED + // P7 OQ-8 added LinkArtifacts::storageBlocks and its own three-field table, in the + // disaggregated build only. The pull/monolith number below is the one G1 measures and + // it has not moved - which is the whole reason the member is guarded. + EXPECT_EQ(CountFields(), 3u); + EXPECT_EQ(CountFields(), 58u); +#else EXPECT_EQ(CountFields(), 57u); +#endif EXPECT_EQ(CountFields(), 8u); } diff --git a/MobileGL/MG_Test/Program/ProgramTest.cpp b/MobileGL/MG_Test/Program/ProgramTest.cpp index 5d838b82..9ea6e2da 100644 --- a/MobileGL/MG_Test/Program/ProgramTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramTest.cpp @@ -380,6 +380,177 @@ void main() { EXPECT_EQ(GetError(), GL_NO_ERROR); } +#if MOBILEGL_BUILD_DISAGGREGATED +// P7 wave 2 package C, OQ-8 (CONTRACT-P7 §5.3): THE ARCHIVE'S STORAGE-BLOCK INDEX SPACE IS +// SPIRV-REFLECT'S, ELEMENT FOR ELEMENT. +// +// This is the equivalence the whole slice rests on. DirectVulkan's resource queries speak an +// index space that used to be DERIVED, by running SPIRV-Reflect over the program's modules - +// per draw on the wire path (MagmaProgramSource) and behind a mutable cache in the monolith +// one (DirectVulkan.cpp's GetProgramResourceCache). It is now PUBLISHED at link time out of +// glslang's reflection instead (ProgramLinkTask::SnapshotStorageBlockIndexSpace). Two +// different producers, one index space: if they ever disagree, +// ProgramFactory::ReflectLayout - which looks blocks up by the name ITS OWN reflect pass +// produced - starts binding storage buffers at the wrong descriptor, silently. +// +// THE REFERENCE IS COMPUTED HERE, not copied as a constant, and it is computed with the same +// algorithm the old consumer used: per module in linked-stage order, STORAGE_BUFFER +// descriptors only, sorted by (normalised name, binding), deduplicated across modules by name. +// Comparing against a hand-written expected list would only pin what the author believed. +// +// THE PROGRAM IS THE ONE THE CONTRACT ASKS FOR: multi-stage (vertex + fragment), with an +// ARRAYED SSBO and an ATOMIC-COUNTER block. Both are the cases where the two producers could +// legitimately have disagreed and neither is exotic: +// * an instance array reflects in glslang as one entry PER ELEMENT ("Blocks[0]", +// "Blocks[1]") and in SPIR-V as ONE descriptor with a count, so the archive has to strip +// the subscript and keep the first element - which is the one whose binding the single +// descriptor reports; +// * an atomic_uint is lowered by the transpiler onto a synthesized gl_AtomicCounterBlock_ +// BUFFER block, so SPIRV-Reflect sees an ordinary storage descriptor for it and the +// backend's space has always contained it - even though GL's own GL_SHADER_STORAGE_BLOCK +// enumeration excludes it. The archive must follow the BACKEND's space, not GL's. +TEST_F(ProgramTest, TheArchivesStorageBlockOrderIsTheOneSpirvReflectProduces) { + char infoLog[1024] = ""; + const char* vsSrc = R"(#version 460 core +layout(std430, binding = 3) buffer Blocks { + uint value; +} blocks[2]; +layout(binding = 0, offset = 0) uniform atomic_uint counter; +layout(std430, binding = 6) buffer VertexOnly { + uint value; +} vertexOnly; +void main() { + atomicCounterIncrement(counter); + blocks[0].value = 1u; + vertexOnly.value = 2u; + gl_Position = vec4(0.0, 0.0, 0.0, 1.0); +} +)"; + const char* fsSrc = R"(#version 460 core +layout(std430, binding = 5) buffer FragmentOnly { + uint value; +} fragmentOnly; +layout(std430, binding = 3) buffer Blocks { + uint value; +} blocks[2]; +layout(location = 0) out vec4 color; +void main() { + fragmentOnly.value = 3u; + color = vec4(float(blocks[1].value), 0.0, 0.0, 1.0); +} +)"; + + const GLuint vs = CreateShader(GL_VERTEX_SHADER); + ShaderSource(vs, 1, &vsSrc, nullptr); + CompileShader(vs); + GLint status = GL_FALSE; + GetShaderiv(vs, GL_COMPILE_STATUS, &status); + GetShaderInfoLog(vs, sizeof(infoLog), nullptr, infoLog); + ASSERT_EQ(status, GL_TRUE) << infoLog; + + const GLuint fs = CreateShader(GL_FRAGMENT_SHADER); + ShaderSource(fs, 1, &fsSrc, nullptr); + CompileShader(fs); + GetShaderiv(fs, GL_COMPILE_STATUS, &status); + GetShaderInfoLog(fs, sizeof(infoLog), nullptr, infoLog); + ASSERT_EQ(status, GL_TRUE) << infoLog; + + const GLuint program = CreateProgram(); + AttachShader(program, vs); + AttachShader(program, fs); + LinkProgram(program); + GetProgramiv(program, GL_LINK_STATUS, &status); + GetProgramInfoLog(program, sizeof(infoLog), nullptr, infoLog); + ASSERT_EQ(status, GL_TRUE) << infoLog; + + auto programObject = MobileGL::MG_State::pGLContext->GetProgramObject(program); + ASSERT_NE(programObject, nullptr); + + // ---- the reference: the old consumer's algorithm, run here over the real modules ---- + const auto normalise = [](const SpvReflectDescriptorBinding& binding) { + const char* raw = binding.name; + if (binding.type_description != nullptr && binding.type_description->type_name != nullptr) { + raw = binding.type_description->type_name; + } + String name = raw ? raw : ""; + const auto suffix = name.find("[0]"); + return suffix == String::npos ? name : name.substr(0, suffix); + }; + std::vector reference; + const auto& spirvs = programObject->GetSpirvReflection().generatedSpirv; + ASSERT_GE(spirvs.size(), 2u) << "the case needs a multi-stage program to be a real test of " + "the cross-stage dedupe"; + for (const auto& spirv : spirvs) { + if (spirv.empty()) continue; + SpvReflectShaderModule module{}; + ASSERT_EQ(spvReflectCreateShaderModule(spirv.size() * sizeof(Uint), spirv.data(), &module), + SPV_REFLECT_RESULT_SUCCESS); + uint32_t count = 0; + ASSERT_EQ(spvReflectEnumerateDescriptorBindings(&module, &count, nullptr), + SPV_REFLECT_RESULT_SUCCESS); + std::vector bindings(count); + if (count != 0) { + ASSERT_EQ(spvReflectEnumerateDescriptorBindings(&module, &count, bindings.data()), + SPV_REFLECT_RESULT_SUCCESS); + } + std::sort(bindings.begin(), bindings.end(), + [&](const SpvReflectDescriptorBinding* lhs, const SpvReflectDescriptorBinding* rhs) { + const String lhsName = lhs ? normalise(*lhs) : String(); + const String rhsName = rhs ? normalise(*rhs) : String(); + if (lhsName != rhsName) return lhsName < rhsName; + return lhs->binding < rhs->binding; + }); + for (const SpvReflectDescriptorBinding* binding : bindings) { + if (binding == nullptr || + binding->descriptor_type != SPV_REFLECT_DESCRIPTOR_TYPE_STORAGE_BUFFER) { + continue; + } + const String name = normalise(*binding); + if (name.empty()) continue; + if (std::find(reference.begin(), reference.end(), name) != reference.end()) continue; + reference.push_back(name); + } + spvReflectDestroyShaderModule(&module); + } + + // ---- what the archive published ---- + std::vector archived; + for (const auto& block : programObject->GetLinkReflection().storageBlocks) { + archived.push_back(block.name); + } + + ASSERT_FALSE(reference.empty()) + << "the reference walk found no storage descriptors at all, so the comparison below " + "would be vacuous - the shaders or the SPIR-V generation changed under this case"; + EXPECT_EQ(archived, reference) + << "LinkArtifacts::storageBlocks is not the index space SPIRV-Reflect produces. " + "GetShaderStorageBlockIndex/Binding hand these indices to ProgramFactory::ReflectLayout, " + "which resolves blocks by the name its own reflect pass produced, so a divergence " + "here binds storage buffers at the wrong descriptor rather than failing loudly."; + + // The arrayed block must appear ONCE, under its stripped name: glslang reflects it per + // element and SPIR-V carries one descriptor, and collapsing the two is the whole reason + // the archive normalises. + EXPECT_EQ(std::count(archived.begin(), archived.end(), String("Blocks")), 1) + << "the arrayed SSBO must collapse to one entry named without its subscript"; + // The atomic counter is in the BACKEND's space even though GL's own enumeration excludes it. + EXPECT_NE(std::find_if(archived.begin(), archived.end(), + [](const String& name) { + return name.rfind("gl_AtomicCounterBlock", 0) == 0; + }), + archived.end()) + << "the synthesized atomic-counter block is a STORAGE_BUFFER descriptor in SPIR-V, so " + "DirectVulkan's index space contains it and the archive has to as well"; + + // And the indices actually resolve, which is what the consumers do with them. + for (Uint i = 0; i < archived.size(); ++i) { + EXPECT_EQ(MG_Backend::DirectVulkan::GetShaderStorageBlockIndex(*programObject, archived[i]), i) + << "name " << archived[i] << " did not resolve to its own index"; + } + EXPECT_EQ(GetError(), GL_NO_ERROR); +} +#endif // MOBILEGL_BUILD_DISAGGREGATED + TEST_F(ProgramTest, DirectVulkanStorageBlockUsesShaderLayoutBinding) { char infoLog[1024] = ""; const char* csSrc = R"(#version 460 core diff --git a/docs/Disaggregated/notes/p7/magma-c.md b/docs/Disaggregated/notes/p7/magma-c.md index 0c818614..9b581d8b 100644 --- a/docs/Disaggregated/notes/p7/magma-c.md +++ b/docs/Disaggregated/notes/p7/magma-c.md @@ -232,3 +232,124 @@ divergence this slice closes. Restored: 10/10. **Lane arithmetic.** The three new `Split` entries carry `integration-magma-split`, so that gating lane grows 73 → 75 (and `-spawn` 52 → 54, `-tcp` 54 → 56). Growth only; G14's name sets are grow-only by rule. + +--- + +## Slice 3 — OQ-8: one reflection archive serves the storage-block consumers (§5.3) + +**What landed.** + +- `LinkArtifacts` gains `Vector storageBlocks` (`{name, binding, + dataSize}`), **`#if MOBILEGL_BUILD_DISAGGREGATED` only** — see G1 below. +- `ProgramLinkTask::SnapshotStorageBlockIndexSpace()` fills it in phase A, right after + `SnapshotGlslangReflection`. +- Its `VisitFields` row and its own three-field table join the codec; + `kProgramArtifactsCodecVersion` 2 → 3; `ProgramArtifactsSchemaFingerprint()` and therefore + `wireFingerprint` move with them, which is expected and is what makes a mixed-version pair + refuse at the handshake rather than at the first program. +- `MagmaProgramSource::EnsureStorageBlocks` is **deleted**, with `m_storageBlocks`, + `m_storageBlocksReady`, the local `StorageBlock` struct and `#include `. +- `DirectVulkan.cpp`'s monolith consumer reads the archive too, under `#if` symmetry. + +**Why phase A and not phase B.** The obvious home is where the SPIR-V exists, and there is +exactly one function that sees both finished halves — `ProgramSpirvTask::RunBody`'s L1 cache +insert — but its writes reach only the TRANSLATION CACHE. Phase A's `artifacts` has already +been moved into the `ProgramObject` by then (`ProgramObject.cpp`'s `JoinPendingLink`), and +phase B is structurally forbidden from touching it. A field filled there would therefore be +present on a cache HIT and absent on every cold link — the worst of both, and invisible until +a second identical program linked. So the fill is glslang-derived, in phase A, and the +equivalence below is what makes that safe. + +**The equivalence is measured, not asserted.** The consumers speak the index space +SPIRV-Reflect produces, and the archive now produces it from glslang's reflection instead. +`ProgramTest.TheArchivesStorageBlockOrderIsTheOneSpirvReflectProduces` runs the OLD algorithm — +SPIRV-Reflect over the real modules, sorted by (normalised name, binding), deduplicated across +stages — over a multi-stage program with an arrayed SSBO and an atomic-counter block, and +compares element for element. Measured reference on this tree: + +``` +{ "Blocks", "VertexOnly", "gl_AtomicCounterBlock_0", "FragmentOnly" } +``` + +which exercises all three of the cases that could have diverged: the instance array collapses +to one entry (glslang reflects per element, SPIR-V carries one descriptor with a count), the +synthesized atomic-counter block IS in the backend's space although GL's own +`GL_SHADER_STORAGE_BLOCK` enumeration excludes it, and `FragmentOnly` lands last because the +cross-stage dedupe walks stages in linked order. + +**MEASUREMENT — `spvReflectCreateShaderModule` on the Magma wire draw path, per frame.** +Same binary, same workload, same window (`frames=1 window=1 draws=1 acc=18`, identical bytes), +`DirectVulkan` × inproc, `StorageBufferRegrowScenario` + the pipeline storage-block case, with +a temporary `MGLOG_I("OQ8_REFLECT_CALL")` at the call site: + +| | reflect calls | frames | draws | +|---|---|---|---| +| BEFORE (`HEAD`'s per-draw `EnsureStorageBlocks`) | **4** | 1 | 1 | +| AFTER (reads the archive) | **0** | 1 | 1 | + +The AFTER number is 0 structurally, not incidentally: the call site is gone and +`MagmaProgramSource.h` no longer includes `spirv_reflect.h` at all, which is the gate a future +edit would have to break on purpose. (A `MagmaProgramSource` is a borrowed view constructed +once per wire draw, so its `mutable` memo was cold every time it was read — that is why the +per-draw cost existed at all.) + +**The monolith half: DONE, under `#if` symmetry — with one caveat the integrator should see.** +The task allowed deferring this if it could not be done without moving pull `.text`. It can, +and it is: `GetShaderStorageBlockIndex` / `GetShaderStorageBlockBinding` / +`ClearProgramResourceCaches` and the block-binding setter each have a disaggregated arm that +reads the archive and a `#else` arm that is the previous code verbatim. In the disaggregated +build that retires `g_programResourceCaches`, `ProgramResourceCache`, `StorageBlockResource`, +`BufferVariableResource`, `AddBufferVariablesRecursive`, `NormalizeDescriptorName`, +`GetProgramResourceCache` **and the rehash hazard** — the open-addressed-map dangling reference +that was a reproducible segfault in `ProgramPipelineScenario`'s two storage-block cases. + +Two things are worth stating plainly: + +1. **The pull build keeps all of it.** The archive member is disaggregated-only because one + more `Vector` in `LinkArtifacts` changes its size, its implicit destructor and its move + constructor, and G1 pins the pull build's `.text` byte for byte. So §5.3's "`g_programResource + Caches` 随之删" is achieved in the disaggregated build and NOT in the pull one. Retiring the + pull half is a one-line change (drop the guard on the member) the day that pin is retired or + the member is moved — a P13 / integrator call, not a wave-2 one. +2. **Nothing outside `DirectVulkan.cpp` read any of the retired machinery.** Measured: + `BufferVariableResource`, `bufferVariables`, `activeVariables` and `StorageBlockResource` + have no reference anywhere else in the tree, and `dataSize` was written and never read — + `GL_BUFFER_VARIABLE` and `GL_ACTIVE_VARIABLES` are answered by + `MG_Impl/GLImpl/Program/ProgramInterface.cpp` out of glslang's own reflection, which is a + different index space. So the two arms differ in exactly the two functions their callers + use and in nothing else. + +One genuine behaviour improvement rides along: the monolith `GetShaderStorageBlockBinding` now +asks `GetShaderStorageBlockBindingOverride` on every read, over an immutable list, where it +used to read a binding patched into a mutable cache. Same answer by a shorter route, and a +rebind can no longer be lost to a cache rebuild that raced a state-version bump. + +**Proof (green).** `ctest -L unit` 2403/2403 (2402 before this slice; +1 is the new case). +`DirectVulkan.*(ProgramPipeline|StorageBufferRegrow)` 60/60 — the **monolith** `DirectVulkan.` +arm and the `Split` / `Spawn` / `Tcp` arms together, which is what says both consumers were +switched. + +**G1 after the archive change:** `.text` `0xa52203` = 10822147 → 10822147 (+0), 27837 → 27837 +defined symbols, **0 added / 0 removed / 0 resized / 0 renamed**, and the file is byte-identical +on disk (19143824 bytes both sides). The `#if` guards did their job; an unguarded call site +caught it once mid-slice (the pull build failed to compile, which is the loud version). + +**RED-ONCE (executed).** `SnapshotStorageBlockIndexSpace` made to publish nothing: + +``` +ProgramTest.cpp:525: Failure + Which is: {} + Which is: { "Blocks", "VertexOnly", "gl_AtomicCounterBlock_0", "FragmentOnly" } +... +58% tests passed, 25 tests failed out of 60 + 3091 DirectVulkan.ProgramPipelineScenario.ComputeAndGraphicsStagesShareOnePipeline (SEGFAULT) + 3092 DirectVulkan.ProgramPipelineScenario.AStageProgramsStorageBlockBindingReaches... (Failed) + 3282 DirectVulkan.StorageBufferRegrowScenario... (Failed) + 4179 DirectVulkan.Split.Buffers.StorageBufferRegrowScenario... (Failed) + 4198 DirectVulkan.Spawn.Buffers.StorageBufferRegrowScenario... (Failed) + 4217 DirectVulkan.Tcp.Buffers.StorageBufferRegrowScenario... (Failed) + 4469 DirectVulkan.Split.ProgramPipelineScenario... (Failed) +``` + +The monolith arm reds beside the three transports, which is the evidence that the monolith +consumer really was switched and not merely edited. Restored: 61/61.