From 6317066add25e9e89fa52b274a951e34cb25b788 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 21 Aug 2026 04:38:59 -0400 Subject: [PATCH] [Fix, Test] (DirectGLES): reach an image array's non-consecutive units by widening the array over their span --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 76 ++++++ MobileGL/MG_Backend/DirectGLES/Utils.cpp | 216 ++++++++++++++++++ MobileGL/MG_Backend/DirectGLES/Utils.h | 45 ++++ .../Scenarios/ImageLoadStoreSsoScenario.cpp | 35 ++- .../Backend/DirectGLES/EsslShaderPassTest.cpp | 168 ++++++++++++++ 5 files changed, 530 insertions(+), 10 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 32d17b19..c5abeea7 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -5357,6 +5357,52 @@ namespace MobileGL::MG_Backend::DirectGLES { return inputs; } + // Every image ARRAY whose elements the application did NOT leave on units consecutive from + // element zero - the only shape ESSL can spell, since an image unit there comes solely + // from the one layout(binding=N) an array declaration carries. Desktop GL assigns them + // per element with glUniform1i, which ES makes an INVALID_OPERATION on an image uniform, + // so there is nothing to fix at the API end and the emitted text has to carry it + // (RemapImageArrayElementUnits). Empty for every program that does not do this, which is + // very nearly all of them - one walk of the reflection and no allocation in that case. + Vector CollectNonConsecutiveImageArrayPlans( + const MG_State::GLState::ProgramObject& stateProgramObject) { + Vector plans; + const Uint maxUniformLoc = stateProgramObject.GetMaxUniformLocation(); + for (Uint loc = 0; loc <= maxUniformLoc; ++loc) { + const auto& name = stateProgramObject.GetUniformName(loc); + if (name.empty()) continue; + if (!IsImageUniformType(stateProgramObject.GetUniformType(loc))) continue; + // Reflection repeats the array's "g_image[0]" spelling at EVERY location the array + // spans, so only the location that name resolves back to is the array itself. + if (stateProgramObject.GetUniformLocation(name) != static_cast(loc)) continue; + const String baseName = ImageUniformBaseName(name); + if (baseName == name) continue; // a scalar image: one binding says it all + + ImageArrayUnitPlan plan; + plan.name = baseName; + for (Uint element = loc; element <= maxUniformLoc && + stateProgramObject.UniformLocationsAliasSameUniform( + static_cast(loc), static_cast(element)); + ++element) { + plan.units.push_back(stateProgramObject.GetUniformSamplerOrImageUnitIndex(element)); + } + if (plan.units.size() < 2) continue; + + Bool consecutive = true; + for (SizeT element = 0; element < plan.units.size(); ++element) { + if (plan.units[element] != plan.units[0] + static_cast(element)) { + consecutive = false; + break; + } + } + // What ESSL does unaided is already right; leaving these out is what keeps the + // emitted text of every ordinary image shader byte-identical to before. + if (consecutive) continue; + plans.push_back(Move(plan)); + } + return plans; + } + Uint64 BackendProgramObjectImpl::ComputeImageUnitFormatSignature() const { if (m_formatlessImageUnits.empty()) return 0; // all but a handful of programs Uint64 signature = 0; @@ -5854,6 +5900,11 @@ namespace MobileGL::MG_Backend::DirectGLES { "different bound formats; left format-less.", conflicted.c_str(), stateProgramObject->GetExternalIndex()); } + // ...and once more for image ARRAYS whose per-element units are not consecutive, which + // ESSL has no way to express in one declaration. Program-wide, like the bake, and read + // from the same snapshot of the reflection; the per-stage rewrite happens below. + const Vector nonConsecutiveImageArrays = + CollectNonConsecutiveImageArrayPlans(*stateProgramObject); // Detach all existing shaders GLint attachedCount = 0; @@ -6184,6 +6235,31 @@ namespace MobileGL::MG_Backend::DirectGLES { // strip, so both halves of a split image inherit the format. source = BakeImageFormatQualifiers(std::move(source), imageFormatBake.esslFormatQualifierByUniformName); + // An image ARRAY whose elements do not sit on consecutive units cannot be spelled + // by the single layout(binding=N) the rebind above stamped: ESSL gives element k + // the unit N+k and there is no glUniform1i to correct it with. Widen the array to + // cover the span and route its subscripts through a constant offset table. AFTER + // the rebind and the format bake, both of which look the array up by its GL + // uniform name and need the binding already there; BEFORE the read+write split, + // so both halves inherit the widened extent and the rebased binding. + if (!nonConsecutiveImageArrays.empty()) { + Vector declinedImageArrays; + source = RemapImageArrayElementUnits( + source, nonConsecutiveImageArrays, + AdvertisedStageImageUniformLimit(shader->GetShaderStage()), &declinedImageArrays); + for (const auto& declined : declinedImageArrays) { + // MGLOG_E, unlatched, like the transpile- and compile-failure diagnostics + // around it: this is the "linked, drew, produced wrong numbers, said + // nothing" shape that cost earlier waves whole days, and one line per + // declined array is bounded by program count. There is no honest GL answer + // to give instead - the frontend has already reported LINK_STATUS = true. + MGLOG_E("Image array %s. Its elements address image units GLSL ES cannot be made to reach " + "from one declaration, so this stage will read and write the WRONG units. State " + "program ID: %u, stage: %s.", + declined.c_str(), stateProgramObject->GetExternalIndex(), + MG_Util::ConvertGLEnumToString(glShaderType).c_str()); + } + } // Wedged between those two on purpose: // * AFTER RebindImageUniformsToFrontendUnits, so the binding it copies onto // both halves of a split image is already the frontend texture unit (and so diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index 8b18c29e..d805ef9e 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -968,6 +968,222 @@ namespace MobileGL::MG_Backend::DirectGLES { } } + namespace { + // The digits of an array extent, or -1 for "not a plain literal size". + Int ParseArrayExtent(const String& text) { + if (text.empty()) return -1; + Int value = 0; + for (const char c : text) { + if (c < '0' || c > '9') return -1; + value = value * 10 + (c - '0'); + if (value > 4096) return -1; // no image array is anywhere near this + } + return value; + } + } // namespace + + String RemapImageArrayElementUnits(const String& glslCode, const Vector& plans, + const Int stageImageUniformBudget, Vector* outDeclined) { +#ifdef TRACY_ENABLE + ZoneScopedC(TRACY_ZONECOLOR_BACKEND); +#endif + if (outDeclined != nullptr) outDeclined->clear(); + if (plans.empty() || glslCode.find("image") == String::npos) return glslCode; + + // Same declaration shape as the split pass reads, with the array extent captured. + static const std::regex imageDeclRegex( + R"(layout\s*\(([^)]*)\)\s*uniform\s+)" + R"(((?:(?:readonly|writeonly|coherent|volatile|restrict|highp|mediump|lowp)\s+)*))" + R"(([iu]?image[A-Za-z0-9_]*)\s+([A-Za-z_][A-Za-z0-9_]*)\s*(?:\[\s*([0-9]*)\s*\])?\s*;)"); + static const std::regex bindingValueRegex(R"(binding\s*=\s*\d+)"); + + struct StageImageDecl { + String name; + String layout; + String qualifiers; + String type; + Int elementCount = 1; + SizeT declStart = 0; + SizeT declLength = 0; + }; + // Every image declaration in the stage, because the budget test below is about the + // stage's total and not about this one array. + Vector decls; + Int stageImageUniforms = 0; + for (std::sregex_iterator it(glslCode.begin(), glslCode.end(), imageDeclRegex), last; it != last; ++it) { + const std::smatch& match = *it; + StageImageDecl decl; + decl.layout = match[1].str(); + decl.qualifiers = NormalizeDeclarationSpacing(match[2].str()); + decl.type = match[3].str(); + decl.name = match[4].str(); + decl.elementCount = match[5].matched ? ParseArrayExtent(match[5].str()) : 1; + decl.declStart = static_cast(match.position(0)); + decl.declLength = match[0].str().size(); + stageImageUniforms += decl.elementCount > 0 ? decl.elementCount : 1; + decls.push_back(Move(decl)); + } + + Vector edits; + Vector takenNames; + for (const ImageArrayUnitPlan& plan : plans) { + const auto decline = [&](const char* why) { + if (outDeclined != nullptr) outDeclined->push_back(plan.name + ": " + why); + }; + if (plan.units.size() < 2) continue; + + const StageImageDecl* decl = nullptr; + for (const auto& candidate : decls) { + if (candidate.name == plan.name) { + decl = &candidate; + break; + } + } + if (decl == nullptr) { + // Absent from this stage entirely is the normal outcome - the reflection is + // program-wide and this pass runs per stage. Named but not RECOGNIZED is not: + // it means the declaration is spelled in some shape the regex above does not + // read, and staying quiet about that is how the wrong units got shipped. + if (ContainsIdentifier(glslCode, plan.name)) { + decline("the stage names it but declares it in a shape this pass cannot read"); + } + continue; + } + if (decl->elementCount < 0 || static_cast(decl->elementCount) != plan.units.size()) { + decline("the emitted array extent disagrees with the reflected element count"); + continue; + } + + Int minUnit = plan.units[0]; + Int maxUnit = plan.units[0]; + Bool consecutive = true; + for (SizeT element = 0; element < plan.units.size(); ++element) { + const Int unit = plan.units[element]; + if (unit < 0) { + minUnit = -1; + break; + } + if (unit != plan.units[0] + static_cast(element)) consecutive = false; + minUnit = std::min(minUnit, unit); + maxUnit = std::max(maxUnit, unit); + } + if (minUnit < 0) { + decline("an element has no image unit"); + continue; + } + // Already exactly what ESSL would do on its own. The caller filters these out; + // repeating the test here keeps the pass correct on its own terms. + if (consecutive) continue; + + const Int spanSize = maxUnit - minUnit + 1; + // The array has to COVER every unit from the lowest to the highest, because ESSL + // hands an array's elements consecutive units and nothing else can move them. + // The elements in between are declared and never accessed; what they cost is + // image-uniform budget, so that is what is checked. + if (stageImageUniformBudget > 0 && + stageImageUniforms - decl->elementCount + spanSize > stageImageUniformBudget) { + decline("the units are spread too far apart to cover within this stage's " + "GL_MAX_*_IMAGE_UNIFORMS"); + continue; + } + + // Every use has to be `name[...]`, or the element index has nowhere to be + // rewritten and the array cannot be widened underneath it. + struct Subscript { + SizeT open; + SizeT close; + }; + Vector subscripts; + Bool everyUseIsSubscripted = true; + for (SizeT pos = glslCode.find(plan.name); pos != String::npos; + pos = glslCode.find(plan.name, pos + 1)) { + if (pos > 0 && IsImagePassIdentifierChar(glslCode[pos - 1])) continue; + const SizeT after = pos + plan.name.size(); + if (after < glslCode.size() && IsImagePassIdentifierChar(glslCode[after])) continue; + if (pos >= decl->declStart && pos < decl->declStart + decl->declLength) { + continue; // the declaration's own name + } + const SizeT open = glslCode.find_first_not_of(" \t\r\n", after); + if (open == String::npos || glslCode[open] != '[') { + everyUseIsSubscripted = false; + break; + } + Int depth = 0; + SizeT scan = open; + for (; scan < glslCode.size(); ++scan) { + if (glslCode[scan] == '[') { + ++depth; + } else if (glslCode[scan] == ']' && --depth == 0) { + break; + } + } + if (scan >= glslCode.size() || open + 1 >= scan) { + everyUseIsSubscripted = false; + break; + } + subscripts.push_back({open, scan}); + } + if (!everyUseIsSubscripted) { + decline("it is reached by something other than a subscript, so there is no element " + "index to rewrite"); + continue; + } + + const String mapName = + MakeImageAliasName(IMAGE_UNIT_MAP_PREFIX, plan.name, glslCode, takenNames); + takenNames.push_back(mapName); + + // The widened declaration, rebased on the lowest unit... + String layout = decl->layout; + const String bindingText = "binding = " + std::to_string(minUnit); + if (std::regex_search(layout, bindingValueRegex)) { + layout = std::regex_replace(layout, bindingValueRegex, bindingText); + } else { + layout = bindingText + (layout.empty() ? String() : ", " + layout); + } + String replacement = "layout(" + layout + ") uniform "; + if (!decl->qualifiers.empty()) { + replacement += decl->qualifiers; + replacement += ' '; + } + replacement += decl->type + " " + plan.name + "[" + std::to_string(spanSize) + "];"; + // ...and the table that turns the application's element index into the offset of + // the unit that element was actually assigned. A const array is a constant + // expression when it is indexed by one, so a shader whose subscripts are literals + // keeps constant subscripts; and when the subscript is a loop counter the lookup + // stays dynamically uniform, which is what GLSL ES 3.20 requires of an image + // array index. + const SizeT elementCount = plan.units.size(); + replacement += "\nconst highp int " + mapName + "[" + std::to_string(elementCount) + "] = int[" + + std::to_string(elementCount) + "]("; + for (SizeT element = 0; element < elementCount; ++element) { + if (element != 0) replacement += ", "; + replacement += std::to_string(plan.units[element] - minUnit); + } + replacement += ");"; + edits.push_back({decl->declStart, decl->declLength, Move(replacement)}); + + // `name[EXPR]` -> `name[[EXPR]]`, by insertion, so EXPR itself is untouched + // however it is spelled. + for (const Subscript& subscript : subscripts) { + edits.push_back({subscript.open + 1, 0, mapName + "["}); + edits.push_back({subscript.close, 0, "]"}); + } + } + if (edits.empty()) return glslCode; + + // Back to front, so an earlier edit's offsets stay valid. Two inserts never share an + // offset: a subscript's opening and closing brackets are distinct positions and a + // declaration is replaced whole. + std::sort(edits.begin(), edits.end(), + [](const ImageSourceEdit& a, const ImageSourceEdit& b) { return a.start > b.start; }); + String result = glslCode; + for (const ImageSourceEdit& edit : edits) { + result.replace(edit.start, edit.length, edit.text); + } + return result; + } + String SplitReadWriteImageUniforms(const String& glslCode, GLenum shaderType, Uint* outSplitCount) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 067827b7..e637d6c3 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -235,6 +235,51 @@ namespace MobileGL::MG_Backend::DirectGLES { // stops being safe to edit by hand. String BakeImageFormatQualifiers(String glslCode, const UnorderedMap& esslFormatByUniformName); String RemoveLayoutBinding(const String& glslCode); + // Prefix of the const element->unit-offset table RemapImageArrayElementUnits declares + // next to a widened image array; the suffix is the array's own name. + constexpr const char* IMAGE_UNIT_MAP_PREFIX = "mg_imageUnitMap_"; + // One image ARRAY whose elements the application pointed at units that are not + // consecutive-from-element-zero. + struct ImageArrayUnitPlan { + String name; // the array's name, exactly as the emitted ESSL declares it + Vector units; // the frontend image unit element k has to reach + }; + // Desktop GL lets an application give each element of an image array an ARBITRARY unit + // (glUniform1i per element). ES has no such call at all - "ES image units come + // exclusively from the layout(binding=N) qualifier" - and one declaration carries one + // binding, so ESSL nails an array's elements to the CONSECUTIVE units N, N+1, N+2, ... + // MobileGL used to stamp element [0]'s unit as the binding and let the rest fall where + // they fell: KHR-GL4x.shader_image_load_store.advanced-sso-simple assigns 0,2,4,6 and + // 1,3,5,7, so its two programs actually addressed 0,1,2,3 and 1,2,3,4 - one layer got the + // wrong value and three were never written, with no GL error and no link log. The same + // defect for SAMPLER arrays was fixed API-side (SubscriptUniformNameForElement); an image + // array has no API side to fix. + // + // Repaired by WIDENING the array to cover every unit from the lowest it needs to the + // highest, rebasing its binding on the lowest, and routing every subscript through a + // `const highp int` table of per-element offsets. The elements in between are declared + // and never accessed. A const array indexed by a constant expression IS one, so literal + // subscripts stay literal; and a table lookup on a dynamically uniform index is itself + // dynamically uniform, which is what GLSL ES 3.20 asks of an image array index - so this + // works whether SPIRV-Cross unrolled the application's loop over the array or not. That + // is the reason for the table rather than one scalar declaration per element: scalars + // cannot be selected by a non-constant index at all. + // + // Declines - leaving the array exactly as it was, and naming it in `outDeclined` for the + // caller to report - when the span will not fit in `stageImageUniformBudget` + // (GL_MAX__IMAGE_UNIFORMS; <= 0 means "cannot say", and then it is not enforced), + // when the emitted extent disagrees with the reflection, or when the array is reached by + // anything other than a subscript. Silence was the whole defect here, so a decline must + // be audible. + // + // Must run AFTER RebindImageUniformsToFrontendUnits and BakeImageFormatQualifiers (both + // key on the GL uniform name and on a binding already being stamped) and BEFORE + // SplitReadWriteImageUniforms (so both halves of a split inherit the widened extent and + // the rebased binding) and RemoveLayoutBinding (which is what preserves image bindings). + // Like them, it is downstream of the L2 shader-translation memo, so the per-program units + // it reads need no entry in BuildEsslTranslationKey. + String RemapImageArrayElementUnits(const String& glslCode, const Vector& plans, + Int stageImageUniformBudget, Vector* outDeclined = nullptr); // Prefix of the writeonly half a read+write image uniform is split into (see // SplitReadWriteImageUniforms); the suffix is the image's own (already stage-tagged) name. constexpr const char* IMAGE_WRITE_ALIAS_PREFIX = "mg_imageWrite_"; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp index d78426cc..fcdb3cf0 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp @@ -127,14 +127,24 @@ void main() // One qualifier is all an ARRAY declaration can carry, and ESSL then gives the // array's elements the CONSECUTIVE units N, N+1, N+2, ... - so a per-element // assignment that is not consecutive (the conformance case uses 0, 2, 4, 6) has no - // spelling in a single declaration and cannot be expressed at all without splitting - // the array into one declaration per element and rewriting every use of it. + // spelling in a single declaration. // - // Scoped rather than disabled, exactly as ProgramPipelineScenario scopes its - // storage-block rebinding cases: the defect is per-backend and the frontend - // mechanism these cases exist for - per-element units surviving the trip to the - // pipeline composite - is fully exercised on Magma. - bool PerElementImageUnitsAreHonoured() const { return Gl().BackendName() == "DirectVulkan"; } + // RemapImageArrayElementUnits repairs it by WIDENING the array to cover every unit + // from the lowest it needs to the highest and routing each subscript through a const + // offset table. What that costs is image-uniform budget: units 0..6 need SEVEN + // fragment image uniforms where the application declared four. So the gate is the + // budget, not the backend - a driver that cannot afford the widening declines it + // (and says so at ERROR level) rather than addressing the wrong units silently. + // DirectVulkan has no such constraint and needs no widening at all. + bool PerElementImageUnitsAreHonoured() const { + if (Gl().BackendName() == "DirectVulkan") return true; + GLint maxFragmentImageUniforms = 0; + glGetIntegerv(GL_MAX_FRAGMENT_IMAGE_UNIFORMS, &maxFragmentImageUniforms); + while (glGetError() != GL_NO_ERROR) { + } + // The widest span either of the two fragment programs below needs: 0..6 and 1..7. + return maxFragmentImageUniforms >= 7; + } // The scenarios below need image load/store at all; a driver without it should skip // rather than fail. @@ -164,7 +174,8 @@ void main() if (!Ready()) return; if (!ImagesAreUsable()) GTEST_SKIP() << "fewer than 8 image units"; if (!PerElementImageUnitsAreHonoured()) { - GTEST_SKIP() << "non-consecutive per-element image units cannot be baked into ESSL"; + GTEST_SKIP() << "fewer than 7 fragment image uniforms: the widening that covers a " + "non-consecutive image array does not fit"; } HeadlessGL& gl = Gl(); @@ -283,8 +294,12 @@ void main() TEST_F(ImageLoadStoreSsoScenario, AnImageArrayAlongsideAnotherDescriptorKeepsBothBindings) { if (!Ready()) return; if (!ImagesAreUsable()) GTEST_SKIP() << "fewer than 8 image units"; - if (!PerElementImageUnitsAreHonoured()) { - GTEST_SKIP() << "non-consecutive per-element image units cannot be baked into ESSL"; + // The defect this guards is the SPIR-V descriptor remap, which only Magma has; the units + // here are consecutive on purpose, so on Espryt this would exercise nothing the case + // above does not. Scoped by what it TESTS rather than by the image-array widening, which + // it deliberately never triggers. + if (Gl().BackendName() != "DirectVulkan") { + GTEST_SKIP() << "the descriptor binding remap under test is DirectVulkan's"; } HeadlessGL& gl = Gl(); diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 5d707419..c27f2f9e 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -19,8 +19,11 @@ using namespace MobileGL; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::BakeImageFormatQualifiers; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ForceFlatIntegerVaryings; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_STAGE_ALIAS_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_UNIT_MAP_PREFIX; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITE_ALIAS_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ImageArrayUnitPlan; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ImageStageAliasPrefix; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemapImageArrayElementUnits; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemoveLayoutBinding; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestExtendedImageFormats; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestViewportArrayExtension; @@ -47,6 +50,7 @@ namespace { // The writeonly half is minted from the ALREADY stage-tagged name, so it carries both. String WriteAlias(const String& name) { return String(IMAGE_WRITE_ALIAS_PREFIX) + name; } String SplitWriteAlias(const String& name) { return WriteAlias(StageAlias(name)); } + String UnitMap(const String& name) { return String(IMAGE_UNIT_MAP_PREFIX) + name; } } // namespace // The bug the pass exists for. SPIRV-Cross speculatively marks every storage image @@ -502,6 +506,170 @@ TEST(SplitReadWriteImageUniformsTest, EveryStageTagIsDistinct) { } } +// --------------------------------------------------------------------------------------- +// RemapImageArrayElementUnits +// +// ES takes an image unit only from layout(binding=N), and one declaration carries one of them, +// so an image array's elements land on N, N+1, N+2, ... Desktop GL lets an application point +// each element wherever it likes with glUniform1i, which ES makes an INVALID_OPERATION on an +// image uniform - there is no API side to fix, so the emitted text has to carry it. + +namespace { + // The advanced-sso-simple shape: a four-element image array on units 0, 2, 4, 6, written + // through a loop counter (which is how SPIRV-Cross emits the conformance case's + // `for (int i = 0; i < g_image.length(); ++i)` when it does not unroll it). + const char* const kSsoImageArrayFS = R"(#version 320 es +layout(rgba32f, binding = 0) uniform writeonly highp image2D g_image[4]; +void main() +{ + for (int i = 0; i < 4; i++) + { + imageStore(g_image[i], ivec2(gl_FragCoord.xy), vec4(1.0)); + } +} +)"; + + ImageArrayUnitPlan Plan(const String& name, const Vector& units) { + ImageArrayUnitPlan plan; + plan.name = name; + plan.units = units; + return plan; + } +} // namespace + +// The defect, end to end. Elements 0..3 need units 0, 2, 4, 6, so the array is widened to cover +// units 0..6 and every subscript is routed through the offset table. Before this, the single +// stamped binding sent the four elements to units 0, 1, 2, 3. +TEST(RemapImageArrayElementUnitsTest, NonConsecutiveUnitsWidenTheArrayAndRouteEverySubscript) { + Vector declined; + const String out = + RemapImageArrayElementUnits(kSsoImageArrayFS, {Plan("g_image", {0, 2, 4, 6})}, 8, &declined); + + EXPECT_TRUE(declined.empty()) << (declined.empty() ? String() : declined[0]); + // Seven elements from binding 0, i.e. units 0..6 - the span the four assigned units need. + EXPECT_TRUE(Contains(out, "layout(rgba32f, binding = 0) uniform writeonly highp image2D g_image[7];")) << out; + EXPECT_TRUE(Contains(out, "const highp int " + UnitMap("g_image") + "[4] = int[4](0, 2, 4, 6);")) << out; + EXPECT_TRUE(Contains(out, "imageStore(g_image[" + UnitMap("g_image") + "[i]], ivec2(gl_FragCoord.xy)")) << out; + // The original four-element extent is gone; nothing may still address units 0,1,2,3. + EXPECT_FALSE(Contains(out, "image2D g_image[4];")) << out; +} + +// The other program of the same conformance case: units 1, 3, 5, 7, so the binding rebases onto +// the LOWEST unit rather than staying on element [0]'s. +TEST(RemapImageArrayElementUnitsTest, TheBindingRebasesOntoTheLowestUnitInTheSpan) { + const String source = R"(#version 320 es +layout(rgba32f, binding = 3) uniform writeonly highp image2D g_image[4]; +void main() +{ + imageStore(g_image[0], ivec2(0), vec4(2.0)); + imageStore(g_image[3], ivec2(0), vec4(2.0)); +} +)"; + const String out = RemapImageArrayElementUnits(source, {Plan("g_image", {3, 1, 7, 5})}, 8); + EXPECT_TRUE(Contains(out, "layout(rgba32f, binding = 1) uniform writeonly highp image2D g_image[7];")) << out; + // Offsets from the new base, in the application's element order - the order is what carries + // the assignment, so it must NOT be sorted. + EXPECT_TRUE(Contains(out, "const highp int " + UnitMap("g_image") + "[4] = int[4](2, 0, 6, 4);")) << out; + // A literal subscript stays a constant expression: a const array indexed by one is one. + EXPECT_TRUE(Contains(out, "imageStore(g_image[" + UnitMap("g_image") + "[0]], ivec2(0)")) << out; + EXPECT_TRUE(Contains(out, "imageStore(g_image[" + UnitMap("g_image") + "[3]], ivec2(0)")) << out; +} + +// Consecutive-from-element-zero is exactly what ESSL does unaided, so the emitted text of an +// ordinary image shader must come out byte-identical. The caller filters these; the pass must +// not depend on that. +TEST(RemapImageArrayElementUnitsTest, ConsecutiveUnitsAreLeftCompletelyAlone) { + const String source = R"(#version 320 es +layout(rgba32f, binding = 2) uniform writeonly highp image2D g_image[3]; +void main() +{ + imageStore(g_image[1], ivec2(0), vec4(1.0)); +} +)"; + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {2, 3, 4})}, 8), source); + // ...and so is a plan for an array this stage does not declare at all: the reflection is + // program-wide, the pass runs per stage. + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("other_image", {0, 4})}, 8), source); +} + +// Widening costs image-uniform budget, and a stage that cannot afford it must be told so rather +// than silently addressing the wrong units - the exact silence this whole pass exists to end. +TEST(RemapImageArrayElementUnitsTest, ASpanThatExceedsTheStageBudgetIsDeclinedAndNamed) { + Vector declined; + const String out = + RemapImageArrayElementUnits(kSsoImageArrayFS, {Plan("g_image", {0, 2, 4, 6})}, 4, &declined); + EXPECT_EQ(out, String(kSsoImageArrayFS)) << out; + ASSERT_EQ(declined.size(), 1u); + EXPECT_TRUE(Contains(declined[0], "g_image")) << declined[0]; + + // A budget of "cannot say" (the ES side reports no limit for this stage) must not be read as + // a budget of zero - that would decline every array on a driver that simply does not answer. + Vector unknownBudget; + const String repaired = + RemapImageArrayElementUnits(kSsoImageArrayFS, {Plan("g_image", {0, 2, 4, 6})}, -1, &unknownBudget); + EXPECT_TRUE(unknownBudget.empty()); + EXPECT_TRUE(Contains(repaired, "image2D g_image[7];")) << repaired; +} + +// A use the pass cannot see a subscript on has no element index to rewrite, so widening the +// array underneath it would change which unit it reaches. Decline, loudly, and change nothing. +TEST(RemapImageArrayElementUnitsTest, AUseWithoutASubscriptIsDeclined) { + const String source = R"(#version 320 es +layout(rgba32f, binding = 0) uniform writeonly highp image2D g_image[2]; +void helper(); +void main() +{ + imageStore(g_image[0], ivec2(0), vec4(1.0)); + helper(g_image); +} +)"; + Vector declined; + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 5})}, 8, &declined), source); + ASSERT_EQ(declined.size(), 1u); + EXPECT_TRUE(Contains(declined[0], "g_image")) << declined[0]; +} + +// The reflection and the emitted text have to be talking about the same array. If they are not, +// the pass has misidentified something and must not rewrite on a guess. +TEST(RemapImageArrayElementUnitsTest, AnExtentThatDisagreesWithTheReflectionIsDeclined) { + const String source = R"(#version 320 es +layout(rgba32f, binding = 0) uniform writeonly highp image2D g_image[2]; +void main() +{ + imageStore(g_image[0], ivec2(0), vec4(1.0)); +} +)"; + Vector declined; + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 4, 8})}, 16, &declined), source); + ASSERT_EQ(declined.size(), 1u); +} + +// The two passes that run after it have to see the widened declaration and keep its binding: an +// ES image unit cannot be assigned through the API, so the qualifier is the only mechanism there +// is, and a read+write array is split into two declarations that must BOTH be the widened one. +TEST(RemapImageArrayElementUnitsTest, TheWidenedArraySurvivesTheLaterImagePasses) { + const String source = R"(#version 320 es +layout(rgba32f, binding = 4) uniform highp image2D g_image[2]; +void main() +{ + imageStore(g_image[1], ivec2(0), imageLoad(g_image[0], ivec2(0))); +} +)"; + String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 6})}, 8); + out = SplitReadWriteImageUniforms(out, kStage); + out = RemoveLayoutBinding(out); + + // Both halves, both widened to the three units 4..6, and both still bound. + EXPECT_EQ(CountOf(out, "binding = 4"), 2u) << out; + EXPECT_TRUE(Contains(out, "image2D " + StageAlias("g_image") + "[3];")) << out; + EXPECT_TRUE(Contains(out, "image2D " + SplitWriteAlias("g_image") + "[3];")) << out; + // The offset table is untouched by the rename - it is not an image uniform - and both halves + // still route their subscripts through it. + EXPECT_TRUE(Contains(out, "const highp int " + UnitMap("g_image") + "[2] = int[2](0, 2);")) << out; + EXPECT_TRUE(Contains(out, SplitWriteAlias("g_image") + "[" + UnitMap("g_image") + "[1]]")) << out; + EXPECT_TRUE(Contains(out, StageAlias("g_image") + "[" + UnitMap("g_image") + "[0]]")) << out; +} + // --------------------------------------------------------------------------------------- // RetargetTextureBufferExtension //