From 257fcbfd0bf51e8522898c0c805247306d134520 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 16:43:29 -0400 Subject: [PATCH] [Fix, Test] (MG_Util, MG_Backend/DirectVulkan, MG_IntegrationTest): review round - one module parse for the shaders with no 1D-array image, per-kind values the combined case can name, and the invariants two shared buffers rest on --- .../DirectVulkan/Renderer/UniformManager.cpp | 13 ++++++- .../Scenarios/ImageTargetKindScenario.cpp | 38 ++++++++++++++----- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 3 +- .../ShaderTranspiler/ShaderCompiler.cpp | 11 +++++- .../SpirvPasses/Lower1DArrayImagesPass.cpp | 25 +++++++++--- .../SpirvPasses/Lower1DArrayImagesPass.h | 21 +++++++--- 6 files changed, 87 insertions(+), 24 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index 6ecbc884..7333a15f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -843,8 +843,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { return false; } - // Sized against the VIEW's format, not the texture's attached one - they may differ by the - // paragraph above, and a range that is not a whole number of the view's texels is invalid. + // Sized from the TEXTURE's attached format even though the view may carry a different + // one. That is not a shortcut: GL requires the shader's format qualifier, the format + // passed to glBindImageTexture and the texture's own internal format to belong to the + // same format CLASS (GL 4.6 core, table 8.27), and every member of a class has the same + // texel size. So the three can disagree on interpretation and never on bytes - which is + // what the range below has to be a whole multiple of. const VkDeviceSize texelSize = static_cast(MG_Util::GetSizedInternalFormatSizeInBytes(internalFormat)); const VkDeviceSize rangeOffset = static_cast(textureBuffer->GetBufferRangeOffset()); @@ -1716,6 +1720,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { // is reachable wherever m_maxBindings is small (it clamps to ~16 on Adreno and Mali), // which is exactly where a 7-element CTS sampler array does not fit the slack. imageInfos.reserve(m_maxBindings + arrayDescriptorExtra); + // Exact, and safe only because it is: BOTH texel kinds (samplerBuffer and imageBuffer) + // refuse descriptor arrays at program creation, so each contributes at most one view and + // the total cannot exceed the binding count. The branches below take the address of + // back(), so making a texel kind array-capable without also giving this the surplus + // imageInfos gets would dangle every pTexelBufferView already recorded in `writes`. texelBufferViews.reserve(m_maxBindings); dynamicOffsets.reserve(programObj.dynamicBindings.size() + uboArrayExtra); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp index 10563ae1..d62539ad 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ImageTargetKindScenario.cpp @@ -204,8 +204,8 @@ namespace MGITest { // Storage plus a full fill with `value`, in the spelling each target kind needs. // Returns 0 - having already reported - when the target could not be created. - GLuint MakeTexture(const TargetKind& kind, bool fill) { - const std::vector texels(static_cast(kExtent) * kExtent * kExtent, kFilledValue); + GLuint MakeTexture(const TargetKind& kind, bool fill, GLuint value = kFilledValue) { + const std::vector texels(static_cast(kExtent) * kExtent * kExtent, value); if (kind.buffer) { GLuint buffer = 0; @@ -441,8 +441,14 @@ namespace MGITest { // needs several kinds in one program - a binding remap that only collides when two image // types share a descriptor set, a per-kind rewrite that is not idempotent across declarations // - and that class of defect is precisely what "each kind passes alone but the case still - // fails" would mean. Each unit is filled with its own INDEX rather than a constant, so the - // sum names how many units contributed and a single mis-bound unit does not cancel out. + // fails" would mean. + // + // Each unit is filled with its own DISTINCT value rather than a shared one, so a shortfall + // names WHICH kind is missing rather than merely how many are: with one shared value, "three + // kinds read zero" and "one kind read zero" differ only by a multiple, and any two kinds are + // interchangeable in the total. A sum still cannot see two kinds SWAPPING - addition is + // commutative, and the conformance case has exactly the same blind spot - but the single-kind + // cases above pin each kind to its own texture already, so a swap cannot hide there. TEST_F(ImageTargetKindScenario, AllKindsInOneProgram) { if (!Ready()) return; if (!ImagesAreUsable()) GTEST_SKIP() << "no compute image uniforms"; @@ -483,12 +489,15 @@ namespace MGITest { const GLuint program = MakeComputeProgram(source); if (program == 0) return; - // Each unit gets its own value, so the sum says how many units contributed. + // Powers of two, so the shortfall's bit pattern names exactly which kinds read zero - + // no other subset of the values can sum to the same total. Eleven kinds at most, so the + // largest is 1 << 10 and the sum cannot approach a uint's range. GLuint expected = 0; for (std::size_t i = 0; i < kinds.size(); ++i) { - const GLuint texture = MakeTexture(kinds[i], true); + const GLuint value = 1u << i; + const GLuint texture = MakeTexture(kinds[i], true, value); if (texture == 0) return; - expected += kFilledValue; + expected += value; glBindImageTexture(static_cast(i), texture, 0, GL_TRUE, 0, GL_READ_ONLY, GL_R32UI); ASSERT_EQ(FirstGLError(), 0u) << kinds[i].name << ": glBindImageTexture errored"; } @@ -504,10 +513,19 @@ namespace MGITest { glMemoryBarrier(GL_ALL_BARRIER_BITS); EXPECT_EQ(FirstGLError(), 0u) << "the dispatch leaked a GL error"; - EXPECT_EQ(ReadResult(ssbo), expected) + const GLuint actual = ReadResult(ssbo); + std::string missing; + for (std::size_t i = 0; i < kinds.size(); ++i) { + if ((actual & (1u << i)) == 0u) { + if (!missing.empty()) missing += ", "; + missing += kinds[i].name; + } + } + EXPECT_EQ(actual, expected) << "the sum over " << kinds.size() - << " image target kinds is wrong; each kind contributes " << kFilledValue - << ", so the shortfall is a whole number of kinds that read zero"; + << " image target kinds is wrong; each kind contributes its own bit, and these read " + "zero: " + << (missing.empty() ? "(none - so some kind read a value it was never given)" : missing); glUseProgram(0); } diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 99a220a2..4577b06f 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -3632,7 +3632,8 @@ void main() { ssb.sum = uint(imageSize(i0).x) + imageLoad(i0, ivec2(0, 0)).r; } )", GL_COMPUTE_SHADER); ASSERT_FALSE(spirv.empty()); - ASSERT_TRUE(Lower1DArrayImagesPass::BinaryQueriesA1DArrayStorageImageSize(spirv)) + const auto traits = Lower1DArrayImagesPass::InspectBinary(spirv); + ASSERT_TRUE(traits.declaresImage && traits.queriesImageSize) << "the fixture must contain the shape the pass declines"; Vector lowered; diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 632845d9..37da152c 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -839,7 +839,16 @@ namespace MobileGL { // MGLOG_I, deliberately: MGLOG_E/W are compiled out at the INFO level every CI, // retrace and release build uses, and this is exactly the diagnostic that has to // survive to explain the shader the driver is about to reject. - if (Lower1DArrayImagesPass::BinaryQueriesA1DArrayStorageImageSize(inputBinary)) { + const auto traits = Lower1DArrayImagesPass::InspectBinary(inputBinary); + // The overwhelmingly common answer, and the reason the inspection exists: no + // 1D-array storage image, so the module is handed back byte for byte without an + // Optimizer ever being built. Every ESSL shader in the process passes through + // here, so the cost of the case with nothing to do is the cost of this pass. + if (!traits.declaresImage) { + outputBinary = inputBinary; + return true; + } + if (traits.queriesImageSize) { MGLOG_I("[spirv] Lower1DArrayImagesForEssl: the module queries the size of a 1D-array " "storage image, which cannot be answered in the 2D-array shape ES stores it in; " "leaving the module alone, and a strict ES driver will reject it"); diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp index f4af03a6..d4373710 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp @@ -112,16 +112,30 @@ namespace MobileGL { } } // namespace - bool Lower1DArrayImagesPass::BinaryQueriesA1DArrayStorageImageSize(const Vector& binary) { + Lower1DArrayImagesPass::ModuleTraits Lower1DArrayImagesPass::InspectBinary(const Vector& binary) { + ModuleTraits traits{}; if (binary.empty()) { - return false; + return traits; } std::unique_ptr context = spvtools::BuildModule( SPV_ENV_VULKAN_1_1, [](spv_message_level_t, const char*, const spv_position_t&, const char*) {}, binary.data(), binary.size()); if (!context) { - return false; + return traits; } + + // The type table settles it for the cheap half, and it is the half almost every + // shader takes: no such type declared, nothing to inspect further. + for (const Instruction& type : context->module()->types_values()) { + if (Is1DArrayStorageImageType(&type)) { + traits.declaresImage = true; + break; + } + } + if (!traits.declaresImage) { + return traits; + } + for (auto& function : *context->module()) { for (auto& block : function) { for (auto& instruction : block) { @@ -130,12 +144,13 @@ namespace MobileGL { } if (Is1DArrayStorageImageType( ResolveImageType(context.get(), instruction.GetSingleWordInOperand(0)))) { - return true; + traits.queriesImageSize = true; + return traits; } } } } - return false; + return traits; } spvtools::opt::Pass::Status Lower1DArrayImagesPass::Process() { diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h index 0db29f9a..c9e67e12 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h @@ -65,11 +65,22 @@ namespace MobileGL { const char* name() const override { return "mobilegl-lower-1d-array-images"; } Status Process() override; - // True when the module declares a 1D-array storage image whose size is queried, - // which is the shape this pass refuses to translate. Checked by the caller before - // running, so a declined module is handed on untouched rather than partly - // rewritten. - static bool BinaryQueriesA1DArrayStorageImageSize(const Vector& binary); + // What one inspection of a binary tells the caller. Both answers come from a + // SINGLE parse on purpose: every ESSL shader in the process reaches this, and + // almost none of them declare a 1D-array storage image, so the common path has to + // cost one module parse and no optimizer run at all - not one parse to ask about + // size queries and a second inside an Optimizer that then early-outs. + struct ModuleTraits { + // The module declares a 1D-array storage image, i.e. there is anything to do. + bool declaresImage = false; + // ...and queries its size, which is the shape this pass refuses to translate: + // afterwards the image is a 2D array, so the query yields three components + // where the shader consumes two, and there is no correct two-component answer + // to substitute. The caller leaves such a module alone rather than half + // rewriting it. + bool queriesImageSize = false; + }; + static ModuleTraits InspectBinary(const Vector& binary); static spvtools::Optimizer::PassToken CreateLower1DArrayImagesPass(); };