From 3c13fa4b4a25bdc5b249931005928e30effa82b5 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 18 Sep 2026 12:07:45 -0400 Subject: [PATCH] [MG_Backend] (Disaggregated): P5e ID-124 - the pull and push flavours compile again Three sites, all pre-existing at 7c6f6886, all the same disease: something guarded by MOBILEGL_BUILD_DISAGGREGATED used from code that is not. Nothing in the gate built either flavour, so nobody knew. - Managers.cpp: StagedTextureTargetForPipeTarget is a pure MGPipeResourceTarget -> TextureTarget switch with no wire, no record and no role in it, but it sat inside a nested #if MOBILEGL_BUILD_DISAGGREGATED while its only caller MGB_TEXPARAM_TARGET is guarded by #if MOBILEGL_PIPE_PUSH alone. Let out of the inner block without moving any text. The narrower fix - guarding the macro block too - is WRONG and was tried: that block also carries TextureDiagName and MGB_TEXTURE_RECORD_ARM_SELECTED, which the push flavour does need, and removing them broke it differently. - DirectGLES.cpp:5575: the storage-block-signature clause called ComputeShaderStorageBlockBindingSignatureOf, declared only under MOBILEGL_PIPE_PUSH, from an UNGUARDED condition list. The clause is push-only in substance too - the signature it compares is over the override map the wire carries. - DirectGLES.cpp BindCurrentTextures: keysMatch was declared inside #if MOBILEGL_BUILD_DISAGGREGATED while the ternary's else-arm and every use sat outside it. The legacy arm becomes a LAZY lambda, not an eager Bool: it reads currentProgram, which is null on the handle arm after pa, so the laziness the ternary gave for free is load-bearing (ID-81 / ID-110 - the arm decides, and the other arm's reads must not happen at all). Gate: flavours rc=0 - pull 915 TUs (push define absent), push 929 TUs (push=1), both arms asserted against compile_commands.json rather than against the flags, because the first version of this step passed -DMOBILEGL_PIPE_PUSH=OFF beside an option that implies it and reported a clean 982-TU "pull" build that was a second copy of the split build. The cache entry still read OFF. Split build unaffected. --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 38 ++++++++++++++----- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 9 +++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 4e37ea3c..f1629990 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -5724,8 +5724,15 @@ namespace MobileGL::MG_Backend::DirectGLES { twin->GetSnormFallbackClampOutputMask() != g_snormFallbackClampOutputMask || twin->GetUnormFallbackClampOutputMask() != g_unormFallbackClampOutputMask || twin->GetFragColorBroadcastCount() != g_fragColorBroadcastCount || +#if MOBILEGL_PIPE_PUSH + // P5e (ID-124): ComputeShaderStorageBlockBindingSignatureOf is declared only + // under MOBILEGL_PIPE_PUSH (Managers.h:3122) and this clause read it from an + // UNGUARDED condition list, so the pull flavour did not compile. The clause is + // push-only in substance too: the signature it compares is over the override + // map the wire carries, and without the wire there is nothing to compare. twin->GetShaderStorageBlockBindingSignature() != ComputeShaderStorageBlockBindingSignatureOf(*currentProgram) || +#endif // A fourth of the same shape, and the reason glBindImageTexture itself does // nothing: GLSL ES demands a format layout qualifier on an image where desktop // GLSL lets a writeonly declaration omit one, so a format-less declaration is @@ -6936,16 +6943,15 @@ namespace MobileGL::MG_Backend::DirectGLES { const SizeT shadowBytes = static_cast(maxTouchedUnit + 1) * sizeof(TextureImpl::g_boundTexturesCache[0]); const Uint64 unitBindingsEpoch = keys.unitBindingsEpoch; -#if MOBILEGL_BUILD_DISAGGREGATED - const Bool keysMatch = - byHandle ? (memo.valid && memo.byHandle && memo.maxTouchedUnit == maxTouchedUnit && - memo.contextSerial == contextSerial && memo.samplerViewsSerial == samplerViewsSerial && - memo.drawProgram.Slot == drawProgram.Slot && memo.drawProgram.Gen == drawProgram.Gen && - memo.shaderCsoSerial == shaderCsoSerial && memo.bindingsSerial == bindingsSerial && - memo.contextGeneration == g_backendContextGeneration) - : -#endif - (memo.valid && memo.glContextId == keys.contextId && + // P5e (ID-124): the legacy key is a LAZY LAMBDA, not an eagerly-computed Bool. It was + // the else-arm of a ternary whose declaration sat inside `#if MOBILEGL_BUILD_DISAGGREGATED` + // while the arm itself and every use of `keysMatch` sat outside it, so neither + // non-disaggregated flavour compiled. Hoisting it to an eager Bool would have been + // WORSE than the build break: it reads `currentProgram`, which is null on the handle + // arm, so the laziness the ternary gave for free is load-bearing (ID-81 / ID-110 - + // the arm decides, and the other arm's reads must not happen at all). + const auto legacyKeysMatch = [&]() -> Bool { + return (memo.valid && memo.glContextId == keys.contextId && memo.maxTouchedUnit == maxTouchedUnit && memo.unitBindingsEpoch == unitBindingsEpoch && memo.samplingResolutionGeneration == keys.samplingGeneration && @@ -6955,6 +6961,18 @@ namespace MobileGL::MG_Backend::DirectGLES { (currentProgram ? currentProgram->GetBackendStateVersion() : 0) && memo.programLinked == (currentProgram && currentProgram->GetLinkStatus()) && memo.contextGeneration == g_backendContextGeneration); + }; +#if MOBILEGL_BUILD_DISAGGREGATED + const Bool keysMatch = + byHandle ? (memo.valid && memo.byHandle && memo.maxTouchedUnit == maxTouchedUnit && + memo.contextSerial == contextSerial && memo.samplerViewsSerial == samplerViewsSerial && + memo.drawProgram.Slot == drawProgram.Slot && memo.drawProgram.Gen == drawProgram.Gen && + memo.shaderCsoSerial == shaderCsoSerial && memo.bindingsSerial == bindingsSerial && + memo.contextGeneration == g_backendContextGeneration) + : legacyKeysMatch(); +#else + const Bool keysMatch = legacyKeysMatch(); +#endif // Short-circuited: the shadow compare is only meaningful once the key (and with it the // snapshotted row count) matches. if (!keysMatch || std::memcmp(memo.boundTextures.data(), TextureImpl::g_boundTexturesCache.data(), diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 5ee85914..63c4a9af 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -2671,6 +2671,14 @@ namespace MobileGL::MG_Backend::DirectGLES { } } +// P5e (ID-124): OUT OF THE DISAGGREGATED BLOCK ON PURPOSE. This is a pure +// MGPipeResourceTarget -> TextureTarget switch with no wire, no record and no role in it, but +// it sat inside the nested `#if MOBILEGL_BUILD_DISAGGREGATED` below while its only caller, +// MGB_TEXPARAM_TARGET, is guarded by `#if MOBILEGL_PIPE_PUSH` alone - so a push build without +// disaggregation did not compile. The narrower fix (guarding the macro block too) is wrong: +// that block also carries TextureDiagName and MGB_TEXTURE_RECORD_ARM_SELECTED, which the push +// flavour does need. Caught by the `flavours` gate step; see ID-124. +#endif // MOBILEGL_BUILD_DISAGGREGATED // The inverse of MG_Pipe::MGPipeResourceTargetForTextureTarget, for the sync's // target reads (ConvertTextureTargetToBackendGLEnum and MapToBackendTextureTarget // both want the frontend enum). @@ -2690,6 +2698,7 @@ namespace MobileGL::MG_Backend::DirectGLES { default: return TextureTarget::Unknown; } } +#if MOBILEGL_BUILD_DISAGGREGATED void Ops_H_TextureSubData(MG_Pipe::MGPipeHandle res, const MG_Pipe::MGPSubData& record, const void* bytes, const MG_Pipe::MGPSubRegion* regions) {