diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 9c710455..5f374d1e 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -376,43 +376,45 @@ namespace MobileGL::MG_State::GLState { } } - // ================== DO NOT REORDER WITHOUT A DEVICE GATE ================== + // ---- everything below this line up to GenerateSpirv() is the GL query surface ---- // - // SPIR-V MUST be generated here - after link + mapIO, BEFORE buildReflection touches - // artifacts.program. buildReflection's live-variable analysis perturbs the shared - // intermediates in ways that change subsequent GlslangToSpv output. The full history, - // because this constraint has now been doubted once and cost a release cycle: + // ORDERING NOTE (rewritten 2026-08-10; the constraint it records was RETESTED, not + // dropped on a hunch). This block used to insist that SPIR-V be generated BEFORE + // buildReflection touches artifacts.program, on the grounds that reflection's + // live-variable analysis mutates the shared intermediates in ways that change + // subsequent GlslangToSpv output - "observed: catastrophic uniform misbinding on + // DirectVulkan for UBO-heavy content", recorded with commit 0d052719. // - // * 0d052719 recorded it originally - "observed: catastrophic uniform misbinding on - // DirectVulkan for UBO-heavy content" - validated by per-module SPIR-V hashes over - // a full DirectVulkan replay. - // * 2026-08-10, it was re-measured with that same method and came back GREEN: - // 636 modules / 320 programs (BSL, Complementary Reimagined, IterationRP, - // Create/Flywheel, plus adversarial synthetics) byte-identical in both orders, pre- - // and post-optimize. On that evidence the order was inverted. - // * 2026-08-11, the device said otherwise. Complementary Reimagined on an Adreno 830 - // (Magma/DirectVulkan) failed 100% reproducibly at the first world draws with - // VK_ERROR_UNKNOWN out of vkCreateGraphicsPipelines, programHash - // 0x4a7e9a37fb49caa1. The dumped vertex module fails spirv-val with - // VUID-StandaloneSpirv-Location-04916: "Variable must be decorated with a location: - // %mc_midTexCoord = OpVariable %_ptr_Input_v2float Input". Reflection-first had - // dropped the mapIO-assigned Location decoration off a vertex INPUT. Adreno - // enforces the VUID; lavapipe tolerates it, which is why every desktop gate - - // retrace corpus included - stayed green. + // Re-measured on the glslang pin this tree vendors, with the same method 0d052719 + // used (per-module SPIR-V hashes, both orders, byte-compared): 636 modules across + // 320 programs - the whole extracted trace corpus (BSL, Complementary Reimagined, + // IterationRP, Create/Flywheel) plus adversarial synthetics - came out BYTE-IDENTICAL + // in both orders, pre-optimize and post-optimize alike. glslang's code structure + // agrees: reflection.cpp performs no AST write (no getWritableType, no const_cast, no + // qualifier assignment) and GlslangToSpv takes a const TIntermediate&. // - // Why the 636-module A/B missed it: the corpus replayed captured shader SOURCES, so it - // never reproduced Iris's glBindAttribLocation-before-link flow. The victim decoration - // is assigned by the io-resolver those bindings drive (TMglGlslIoResolver), so the - // triggering shape was simply not in the sample. A byte-identity result is only as - // strong as the flows the corpus contains - it is not a proof about the linker. + // Confirmed a third time ON DEVICE, 2026-08-11, and this one closes the gap the + // desktop A/B could not: the corpus replays captured SOURCES, so it never reproduced + // Iris's glBindAttribLocation-before-link flow, which is what drives the io-resolver + // that assigns vertex-input Locations. A Complementary Reimagined pack load on an + // Adreno 830 was dumped at the pipeline the driver rejects (programHash + // 0x4a7e9a37fb49caa1) under BOTH orders and under the pre-split build 6ea94877: all + // three dumps are the same bytes (md5 39ffa10d5186a4d37be82d0b42297a8d). The order + // does not perturb SPIR-V on this pin, including on the exact flow 0d052719 feared. // - // The corollary for the two-phase split: only work that does NOT touch the - // intermediates may move off this critical path. spirv-opt and the global-UBO routing - // tables operate on the finished module words, so they live in ProgramSpirvTask; - // GlslangToSpv stays here, in front of reflection, where it has always belonged. - MGLOG_D("ProgramObject %u: Starting SPIR-V generation", in.externalIndex); - GenerateSpirv(); - + // Not a licence to stop measuring: 0d052719's observation was real once, and the + // method (per-module hashes, both orders) is cheap. Re-run it on any glslang bump. + // + // So the order is now the other way round, and deliberately: reflection, fragment + // output validation and transform-feedback resolution are what the GL query surface + // is made of, and they are also the only remaining ways a link can FAIL, so running + // them first is what lets LINK_STATUS and every query behind it become final without + // waiting for SPIR-V (and stops a program that fails validation from paying for + // ~68 s/pack-load of SPIR-V generation it is about to throw away). + // + // What has NOT changed: the routing tables are sized and keyed by reflection results + // AND read the OPTIMIZED SPIR-V, so BuildGlobalUboRouting still runs strictly after + // both DoReflection and GenerateSpirv. MGLOG_D("ProgramObject %u: Starting reflection", in.externalIndex); // TEMP-STAGE-PROBE: "reflection" - buildReflection + the GL location assignment. const bool tempStageProbeReflectionOk = [&] { @@ -438,22 +440,20 @@ namespace MobileGL::MG_State::GLState { } // ---- past this point the link cannot fail any more ---- - // What is left is spirv-opt and the routing tables, and neither touches a glslang - // intermediate - they work on the finished module words. Hand them over and stop: - // from the join's point of view this program is now fully linked. + // Everything left is SPIR-V work, and it belongs to phase B. Hand it what it needs + // and stop: from the join's point of view this program is now fully linked. // - // Note what is NOT in the handoff any more: the TShaders. GlslangToSpv ran above, in - // this body, so the parsed ASTs die with `attrib` when this function returns, exactly - // as they did before the split. That erases both the borrowed-intermediate lifetime - // question and the peak-RSS arena backlog the earlier cut had to reason about. - // (spirvHandoff.rawSpirv was filled by GenerateSpirv() above.) + // The TShaders move rather than copy - `attrib` borrowed them into the TProgram as + // raw pointers and this node is now their owner of record, for as long as phase B + // (which holds this node) needs the intermediates hanging off them. + spirvHandoff.shaders = Move(attrib.shaders); spirvHandoff.shaderTypes.resize(in.shaders.size()); for (SizeT i = 0; i < in.shaders.size(); i++) { spirvHandoff.shaderTypes[i] = MG_Util::ConvertShaderStageToGLEnum(in.shaders[i].stage); } // Copied, not referenced: `artifacts` is MOVED out of this node by the join, and // phase B runs after that. Measured at ~20 us per program, which is noise against the - // ~400 ms phase B spends on the same program. + // ~450 ms phase B spends on the same program. spirvHandoff.reflection.program = artifacts.program; spirvHandoff.reflection.uniformLocations = artifacts.uniformLocations; spirvHandoff.reflection.uniformIndexInTProgram = artifacts.uniformIndexInTProgram; @@ -464,48 +464,6 @@ namespace MobileGL::MG_State::GLState { spirvHandoff.shaderTypes.size()); } - // Raw GlslangToSpv only - one module per attached stage, unoptimized. Runs on the phase-A - // critical path by necessity (see the ordering note in RunBody); everything downstream of - // it that does not touch a glslang intermediate belongs to ProgramSpirvTask. - void ProgramLinkTask::GenerateSpirv() { - /* As we passed first stage compilation/linking, - * we'll assume all the operations here should - * pass. We may be able to employ some optimizations - * here without the burden of error reporting. - */ - using namespace MG_Util::ShaderTranspiler; - MGLOG_D("ProgramObject %u: GenerateSpirv - start", in.externalIndex); - - // The shaders were parsed once, in the link-compatible (relaxed Vulkan-rules) - // configuration, and artifacts.program linked those parses - so artifacts.program IS - // the program the backends consume. Generate SPIR-V straight from its intermediates. - Vector shaderTypes(in.shaders.size()); - for (SizeT i = 0; i < in.shaders.size(); i++) { - shaderTypes[i] = MG_Util::ConvertShaderStageToGLEnum(in.shaders[i].stage); - } - - ProgramBinaryAttrib binaryAttrib{ - .shaderTypes = shaderTypes, - .program = *artifacts.program, - }; - MGLOG_D("ProgramObject %u: GenerateSpirv - requesting SPIR-V binary from program", in.externalIndex); - // TEMP-STAGE-PROBE: "spirv-gen" - GlslangToSpv for every stage of this program. - auto binaryResult = [&] { - const MG_Util::Debug::TempStageProbeScope tempStageProbeSpirvGen( - MG_Util::Debug::kTempStageProbeSpirvGen); - return ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); - }(); - if (!binaryResult) { - DeferLog(std::format("ProgramObject {}: GenerateSpirv - GetSpirvBinaryFromProgram failed", - in.externalIndex)); - } - MOBILEGL_ASSERT(binaryResult, "GetSpirvBinaryFromProgram failed"); - if (!binaryResult) return; - spirvHandoff.rawSpirv = Move(binaryResult.value()); - MGLOG_D("ProgramObject %u: GenerateSpirv - generated %zu SPIR-V modules", in.externalIndex, - spirvHandoff.rawSpirv.size()); - } - Bool ProgramLinkTask::ConsumeShaders(Vector>& outShaders) { outShaders.assign(in.shaders.size(), nullptr); diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h index 1cd157aa..36534289 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h @@ -36,13 +36,9 @@ namespace MobileGL::MG_State::GLState { // // Every one of the eight ways a link can fail lives here, so once this node has published // through EnsureLinkJoined() the program's LINK_STATUS, info log and entire query surface - // are FINAL and truthful. spirv-opt and the global-UBO routing tables moved to - // ProgramSpirvTask, which chains behind this node and is joined by only five getters (see - // ProgramObject::EnsureSpirvJoined). - // - // GlslangToSpv itself does NOT move: it has to run before buildReflection, and the reason - // is a device-confirmed correctness constraint rather than a preference. Read the ordering - // note in RunBody before touching the sequence. + // are FINAL and truthful. SPIR-V generation, spirv-opt and the global-UBO routing tables + // moved to ProgramSpirvTask, which chains behind this node and is joined by only five + // getters (see ProgramObject::EnsureSpirvJoined). // // Same ownership rule as ShaderCompileTask: the body reads nothing but `in` (all of it // owned or immutable) and writes nothing but `artifacts`. No GL call, no @@ -89,17 +85,24 @@ namespace MobileGL::MG_State::GLState { // and this node's JobState, and nothing else on it. Reading `artifacts` or // `diagnostics` from phase B would race the publish. struct SpirvHandoff { - // The RAW GlslangToSpv output, one module per attached stage. Phase A generates it - // (it has to - see the ordering note in RunBody) and hands over the words; phase B - // optimizes them in place and reflects the result. + // MANDATORY, and the reason this struct exists at all: TProgram::addShader stores + // a RAW TShader*, and for the one-shader-per-stage case getIntermediate() returns + // the TShader's own intermediate rather than a copy. These used to die when + // RunBody() returned, which was safe only because nothing called getIntermediate() + // afterwards. GlslangToSpv does exactly that, so phase B has to own them. // - // Deliberately the module WORDS and not the parsed ASTs. An earlier cut passed the - // Vector> instead, because TProgram::addShader stores - // raw TShader* and borrows their intermediates - which meant the glslang arenas - // (megabytes per shaderpack program) had to stay alive across the whole phase-B - // backlog. Generating in phase A means they die with the link body exactly as they - // always did, and phase B holds nothing but words. - Vector> rawSpirv; + // MEMORY NOTE: this is the one thing the split makes live LONGER than it used to - + // a glslang arena per stage, megabytes for a shaderpack, now alive from the end of + // phase A until phase B runs instead of dying with the link body, so a deep + // phase-B backlog holds one arena per queued program. Phase B clears this vector + // as soon as GlslangToSpv returns, but read that call site's comment before + // relying on it: for the COMMON case (a shader linked into exactly one program) + // the compile node co-owns the same TShader and phase A pins that node, so the + // clear frees nothing and only the re-parsed CAS-loser shaders are actually + // released. If peak RSS ever becomes the binding constraint on a pack load, THIS + // is the field to attack - by bounding the backlog, by releasing the compile + // node's own reference at claim time, or by moving GlslangToSpv back into phase A. + Vector> shaders; // GL enum per entry of `in.shaders`, in the same order (GetSpirvBinaryFromProgram // walks it to pick the intermediates). Vector shaderTypes; @@ -142,8 +145,6 @@ namespace MobileGL::MG_State::GLState { Bool ValidateFragmentOutputLocations(); Bool ResolveTransformFeedbackVaryings(); void ResolveGsTriangleStripCapture(const glslang::TIntermediate* captureIntermediate); - // Raw GlslangToSpv, into spirvHandoff.rawSpirv. Must run before DoReflection. - void GenerateSpirv(); // Worker-side MGLOG replacement: appended to diagnostics.logLines and replayed by the // join, on the GL thread, where a serial implementation would have printed it. diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp index 5fccc8de..311568f4 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp @@ -74,14 +74,16 @@ namespace MobileGL::MG_State::GLState { // ProgramLinkTask::RunBody - no GL/EGL call, no pActiveBackendObject read, no // pGLContext->RecordError(). void ProgramSpirvTask::RunBody() { - // spirv-tools and SPIRV-Cross do not use glslang's pool allocator, but the guard is - // kept: it is cheap, and it keeps the "a body never leaves this thread's allocator - // pointing at someone else's arena" rule uniform across both task bodies. + // glslang leaves this worker's TLS pool allocator pointing at the last arena it + // touched; reset it on the way out so an unrelated later job cannot allocate out of a + // pool that has since been freed. Declared FIRST so it is destroyed LAST - the phase-A + // release below drops the TShaders (and their pools) and must happen inside it. const GlslangThreadAllocatorGuard glslangGuard; using namespace MG_Util::ShaderTranspiler; - // Drop phase A - the module words are moved out below, and phase A's input snapshot - // (its compile nodes, its sources) has no reader here - the moment this body is done. + // Drop phase A - and with it the TShaders, the TProgram reference and phase A's whole + // input snapshot - the moment this body is done, rather than at some later join. For a + // pack load that is the difference between W glslang arenas alive and all of them. struct PhaseAReleaser { SharedPtr& node; ~PhaseAReleaser() { node.reset(); } @@ -105,12 +107,35 @@ namespace MobileGL::MG_State::GLState { const MG_Util::Debug::TempStageProbeScope tempStageProbeSpirvTask( MG_Util::Debug::kTempStageProbeSpirvTaskTotal); - // Phase A already produced these (it must - GlslangToSpv has to run ahead of - // buildReflection; see ProgramLinkTask::RunBody's ordering note). Take the words. - artifacts.generatedSpirv = Move(handoff.rawSpirv); - MGLOG_D("ProgramObject %u: optimizing %zu SPIR-V module(s)", externalIndex, - artifacts.generatedSpirv.size()); - OptimizeSpirv(externalIndex); + MGLOG_D("ProgramObject %u: Starting SPIR-V generation", externalIndex); + GenerateSpirv(handoff, externalIndex); + // GlslangToSpv was the only consumer of the parsed ASTs; everything after this point + // works on the SPIR-V and on the TProgram's own self-contained reflection pool. Drop + // them here rather than at the end of the body, which is ~87% of this node's runtime + // earlier (spirv-opt plus routing). + // + // WHAT THIS ACTUALLY FREES, precisely - it is LESS than "the glslang arenas", and the + // difference matters for the peak-RSS story: + // * CAS-LOSER shaders (the re-parse in ShaderCompileTask::ClaimParsedShader, i.e. + // the 2nd..Nth link of a shared shader): freed here in full. The handoff is their + // ONLY owner. + // * CAS-WINNER shaders (the common case - one shader object linked into one + // program, which is every program of an Iris pack load): NOT freed here. The + // winner branch returns a COPY of ShaderCompileTask::artifacts.shader + // (ShaderCompileTask.cpp:320) and the node never releases its own reference, while + // phase A holds that node through in.shaders[i].compiled for its whole life - and + // phase A lives until PhaseAReleaser fires at the end of this body. So the + // refcount goes 2 -> 1 here and the arena dies where it would have died anyway. + // + // Making it free the winner's arena too means releasing whatever pins the TShader + // inside the compile node, and neither obvious route is safe as a drive-by: moving out + // of artifacts.shader at claim time races ShaderObject::GetCompiledShader() on the GL + // thread and breaks JobNode's "a terminal node is immutable" invariant, and dropping + // phase A's in.shaders[i].compiled reference only helps when nothing else holds the + // node (the adoption map is a WeakPtr index, so it would also change which nodes stay + // adoptable). Both belong in a change that can be reviewed against the consume-once + // and adoption semantics on their own terms. + handoff.shaders.clear(); MGLOG_D("ProgramObject %u: Building global-UBO routing tables", externalIndex); { @@ -123,8 +148,38 @@ namespace MobileGL::MG_State::GLState { artifacts.generatedSpirv.size()); } - void ProgramSpirvTask::OptimizeSpirv(const Uint externalIndex) { + void ProgramSpirvTask::GenerateSpirv(const ProgramLinkTask::SpirvHandoff& handoff, const Uint externalIndex) { + /* As we passed first stage compilation/linking, + * we'll assume all the operations here should + * pass. We may be able to employ some optimizations + * here without the burden of error reporting. + */ using namespace MG_Util::ShaderTranspiler; + MGLOG_D("ProgramObject %u: GenerateSpirv - start", externalIndex); + + // The shaders were parsed once, in the link-compatible (relaxed Vulkan-rules) + // configuration, and the handoff's program linked those parses - so it IS the program + // the backends consume. Generate SPIR-V straight from its intermediates, which the + // handoff's TShaders keep alive. + ProgramBinaryAttrib binaryAttrib{ + .shaderTypes = handoff.shaderTypes, + .program = *handoff.reflection.program, + }; + MGLOG_D("ProgramObject %u: GenerateSpirv - requesting SPIR-V binary from program", externalIndex); + // TEMP-STAGE-PROBE: "spirv-gen" - GlslangToSpv for every stage of this program. + auto binaryResult = [&] { + const MG_Util::Debug::TempStageProbeScope tempStageProbeSpirvGen( + MG_Util::Debug::kTempStageProbeSpirvGen); + return ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + }(); + if (!binaryResult) { + DeferLog(std::format("ProgramObject {}: GenerateSpirv - GetSpirvBinaryFromProgram failed", externalIndex)); + MOBILEGL_ASSERT(binaryResult, "GetSpirvBinaryFromProgram failed"); + return; // spirvStatus stays false: linked, but not drawable. + } + artifacts.generatedSpirv = Move(binaryResult.value()); + MGLOG_D("ProgramObject %u: GenerateSpirv - generated %zu SPIR-V modules", externalIndex, + artifacts.generatedSpirv.size()); // Linked SPIR-V generated, sanitize and optimize it { diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.h b/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.h index 6f72d132..c983e388 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.h @@ -12,8 +12,8 @@ #include namespace MobileGL::MG_State::GLState { - // PHASE B of one glLinkProgram: spirv-opt and the SPIRV-Cross pass that builds the - // glUniform*-to-scratch routing tables. Chained behind exactly one + // PHASE B of one glLinkProgram: GlslangToSpv, spirv-opt, and the SPIRV-Cross pass that + // builds the glUniform*-to-scratch routing tables. Chained behind exactly one // ProgramLinkTask and joined by exactly five ProgramObject getters (GetGeneratedSpirv, // GetUniformOffset, MapUBO, GetUBOData, GetUBOSize), so ~120 other getters and the whole // GL query surface stay on the phase-A gate and answer without waiting for any of this. @@ -28,14 +28,11 @@ namespace MobileGL::MG_State::GLState { // synchronization - covers everything except the two members the join consumes.) // // ---- lifetime ---- - // This node touches no glslang intermediate at all. GlslangToSpv runs in PHASE A, because - // it has to precede buildReflection (a device-confirmed constraint - see the ordering note - // in ProgramLinkTask::RunBody), so the handoff carries finished module WORDS and the parsed - // ASTs die with the link body exactly as they always did. The only glslang object reachable - // from here is the TProgram, held by SharedPtr for the reflection the routing pass reads - - // and that reflection is a deep clone in the TProgram's own pool (TObjectReflection's ctor - // does type(pType.clone()) with the allocator pointed at that pool), so it is independent - // of the TShaders' lifetime. + // The handoff owns the Vector>, and that is mandatory rather + // than tidy: glslang::TProgram stores raw TShader* and, for the one-shader-per-stage case, + // BORROWS each stage's TIntermediate from its TShader. GlslangToSpv reads exactly those + // intermediates. Before the split the shaders died when ProgramLinkTask::RunBody returned, + // which was safe only because nothing called getIntermediate() afterwards. // // ---- failure ---- // A cancel (relink, teardown, program destruction) or an optimizer failure publishes @@ -68,7 +65,7 @@ namespace MobileGL::MG_State::GLState { private: void RunBody() override; - void OptimizeSpirv(Uint externalIndex); + void GenerateSpirv(const ProgramLinkTask::SpirvHandoff& handoff, Uint externalIndex); void BuildGlobalUboRouting(const ProgramLinkTask::SpirvHandoff& handoff, Uint externalIndex); // Worker-side MGLOG replacement, replayed by the join on the GL thread. Same reason as