diff --git a/CMakeLists.txt b/CMakeLists.txt index 4c357c43..e6e6259c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -303,7 +303,7 @@ set(SOURCE_FILES MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripNoPerspectivePass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EmulateNoPerspectivePass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeFragmentOutputIndexPass.cpp - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp + MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.cpp MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.cpp MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index cf8a4134..c7c2bfd8 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -5675,7 +5675,7 @@ namespace MobileGL::MG_Backend::DirectGLES { // from the image's own Dim), so the pass moves the type to 2D and widens coordinate, // offset and gradients together. // - // NO KEY MATERIAL, by the same test LegalizeStorageBlockArrayIndexingForEssl passes: + // NO KEY MATERIAL, by the same test LegalizeResourceArrayIndexingForEssl passes: // it takes the module and nothing else, no capability bit arms it, and it self-gates // on the module's own content (BinaryHasOffsetOrGrad1DSampledImage). The module is // already the largest thing in the L2 key, so it is covered completely. @@ -5765,24 +5765,31 @@ namespace MobileGL::MG_Backend::DirectGLES { effectiveSpirv = &outputIndexSpirv; } - // Same rule, different resource, every stage: GL 4.3 lets an array of storage - // blocks be indexed with any dynamically-uniform expression, GLSL ES keeps the - // ES 3.1 constant-expression rule, and the Qualcomm compiler enforces it + // Same rule, two more resources, every stage: desktop GL lets an array of + // storage blocks and an array of image uniforms be indexed with any + // dynamically-uniform expression, GLSL ES keeps the ES 3.1 + // constant-expression rule for both, and the drivers enforce it - Qualcomm // ("indexing into an SSBO array using a non-constant expression is not - // permitted") - losing the stage, the program, and every dispatch that used - // it, while the frontend keeps reporting the link glslang performed. Fold or - // lower the index here, on the ESSL path only: the same module is legal for - // DirectVulkan, which binds the array as one descriptor array. + // permitted"), Mesa ("image arrays indexed with non-constant expressions are + // forbidden in GLSL ES") - losing the stage, the program, and every draw or + // dispatch that used it, while the frontend keeps reporting the link glslang + // performed. Fold or lower the index here, on the ESSL path only: the same + // module is legal for DirectVulkan, which binds the array as one descriptor + // array. + // + // The image half is also what makes RemapImageArrayElementUnits below possible + // at all: that pass rewrites `g_image[k]` into a per-element declaration, and it + // can only do that once every k the emitted ESSL spells is a literal. // // NO KEY MATERIAL, and that is a conclusion rather than an omission: this takes the // module and nothing else - no capability bit arms it, no per-program plan steers // it - and it self-gates on the module's own content - // (BinaryHasDynamicStorageBlockArrayIndexing). The module is already the largest + // (BinaryHasDynamicResourceArrayIndexing). The module is already the largest // thing in the L2 key, so it is fully covered. Contrast LowerViewportIndexForEssl, // whose signature is equally module-only but which SupportsViewportArray ARMS - // that bit is in the key precisely because of it. Vector blockArrayIndexSpirv; - if (MG_Util::ShaderTranspiler::ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl( + if (MG_Util::ShaderTranspiler::ShaderCompiler::LegalizeResourceArrayIndexingForEssl( *effectiveSpirv, blockArrayIndexSpirv, enableSpirvValidation) && !blockArrayIndexSpirv.empty()) { effectiveSpirv = &blockArrayIndexSpirv; @@ -6400,16 +6407,15 @@ namespace MobileGL::MG_Backend::DirectGLES { 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. + // the unit N+k and there is no glUniform1i to correct it with. Split the array + // into one scalar declaration per element, each with its own binding. 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 an + // element that is both read and written is split with its own binding on it. if (!nonConsecutiveImageArrays.empty()) { Vector declinedImageArrays; - source = RemapImageArrayElementUnits( - source, nonConsecutiveImageArrays, - AdvertisedStageImageUniformLimit(shader->GetShaderStage()), &declinedImageArrays); + source = RemapImageArrayElementUnits(source, nonConsecutiveImageArrays, + &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 diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index aaffdcb2..b9af004c 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -1045,8 +1045,9 @@ 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) { + // The digits of an array extent or of an element subscript, or -1 for "not a plain + // decimal literal". + Int ParseNonNegativeIntLiteral(const String& text) { if (text.empty()) return -1; Int value = 0; for (const char c : text) { @@ -1059,7 +1060,7 @@ namespace MobileGL::MG_Backend::DirectGLES { } // namespace String RemapImageArrayElementUnits(const String& glslCode, const Vector& plans, - const Int stageImageUniformBudget, Vector* outDeclined) { + Vector* outDeclined) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif @@ -1082,10 +1083,9 @@ namespace MobileGL::MG_Backend::DirectGLES { 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. + // Every image declaration in the stage; the plans are program-wide and name arrays + // this stage may not declare at all. 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; @@ -1093,10 +1093,9 @@ namespace MobileGL::MG_Backend::DirectGLES { 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.elementCount = match[5].matched ? ParseNonNegativeIntLiteral(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)); } @@ -1130,20 +1129,17 @@ namespace MobileGL::MG_Backend::DirectGLES { continue; } - Int minUnit = plan.units[0]; - Int maxUnit = plan.units[0]; Bool consecutive = true; + Bool everyElementHasAUnit = true; for (SizeT element = 0; element < plan.units.size(); ++element) { const Int unit = plan.units[element]; if (unit < 0) { - minUnit = -1; + everyElementHasAUnit = false; break; } if (unit != plan.units[0] + static_cast(element)) consecutive = false; - minUnit = std::min(minUnit, unit); - maxUnit = std::max(maxUnit, unit); } - if (minUnit < 0) { + if (!everyElementHasAUnit) { decline("an element has no image unit"); continue; } @@ -1151,26 +1147,21 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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; + // Every use has to be `name[]`. The literal is what the split turns + // into a name, and by the time this runs there is always one: + // LegalizeResourceArrayIndexingForEssl has already folded or lowered every + // dynamic image-array subscript in the module, because ESSL forbids one + // outright ("image arrays indexed with non-constant expressions are forbidden + // in GLSL ES"). A subscript that is still an expression here is therefore a + // stage that was never going to compile, and guessing which element it meant + // would only change which unit it addressed wrongly. + struct ElementUse { + SizeT start; // the first character of the name + SizeT length; // through the closing ']' + SizeT element; }; - Vector subscripts; - Bool everyUseIsSubscripted = true; + Vector uses; + const char* refusal = nullptr; 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; @@ -1181,7 +1172,8 @@ namespace MobileGL::MG_Backend::DirectGLES { } const SizeT open = glslCode.find_first_not_of(" \t\r\n", after); if (open == String::npos || glslCode[open] != '[') { - everyUseIsSubscripted = false; + refusal = "it is reached by something other than a subscript, so there is no " + "element index to rewrite"; break; } Int depth = 0; @@ -1194,63 +1186,71 @@ namespace MobileGL::MG_Backend::DirectGLES { } } if (scan >= glslCode.size() || open + 1 >= scan) { - everyUseIsSubscripted = false; + refusal = "it is reached by something other than a subscript, so there is no " + "element index to rewrite"; break; } - subscripts.push_back({open, scan}); + const Int element = ParseNonNegativeIntLiteral( + NormalizeDeclarationSpacing(glslCode.substr(open + 1, scan - open - 1))); + if (element < 0 || element >= decl->elementCount) { + refusal = "its subscript is not a literal element index, so which unit the " + "access reaches cannot be decided here"; + break; + } + uses.push_back({pos, scan + 1 - pos, static_cast(element)}); } - if (!everyUseIsSubscripted) { - decline("it is reached by something other than a subscript, so there is no element " - "index to rewrite"); + if (refusal != nullptr) { + decline(refusal); 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. + // One SCALAR declaration per element, each carrying its own binding. ESSL nails + // an ARRAY's elements to consecutive units and offers no way to move them, so + // the only spelling that reaches an arbitrary set of units is one declaration + // per unit - and with every subscript a literal, every use has exactly one of + // them to be rewritten to. + // + // It costs precisely the image uniforms the application declared, which is why + // there is no budget test here: an array of four elements becomes four scalars + // however far apart their units are. const SizeT elementCount = plan.units.size(); - replacement += "\nconst highp int " + mapName + "[" + std::to_string(elementCount) + "] = int[" + - std::to_string(elementCount) + "]("; + Vector elementNames; + String replacement; for (SizeT element = 0; element < elementCount; ++element) { - if (element != 0) replacement += ", "; - replacement += std::to_string(plan.units[element] - minUnit); + const String elementName = + MakeImageAliasName(IMAGE_ARRAY_ELEMENT_PREFIX, + plan.name + "_" + std::to_string(element), glslCode, takenNames); + takenNames.push_back(elementName); + elementNames.push_back(elementName); + + String layout = decl->layout; + const String bindingText = "binding = " + std::to_string(plan.units[element]); + if (std::regex_search(layout, bindingValueRegex)) { + layout = std::regex_replace(layout, bindingValueRegex, bindingText); + } else { + layout = bindingText + (layout.empty() ? String() : ", " + layout); + } + if (element != 0) replacement += '\n'; + replacement += "layout(" + layout + ") uniform "; + if (!decl->qualifiers.empty()) { + replacement += decl->qualifiers; + replacement += ' '; + } + replacement += decl->type + " " + elementName + ";"; } - 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, "]"}); + // `name[k]` -> the scalar declared for element k, subscript and all. + for (const ElementUse& use : uses) { + edits.push_back({use.start, use.length, elementNames[use.element]}); } } 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. + // Back to front, so an earlier edit's offsets stay valid. No two edits overlap: each + // one covers either a whole declaration or a whole `name[k]`, the declaration's own + // name is skipped when the uses are collected, and one occurrence of a name yields at + // most one edit. std::sort(edits.begin(), edits.end(), [](const ImageSourceEdit& a, const ImageSourceEdit& b) { return a.start > b.start; }); String result = glslCode; diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 6f3a2d18..ea9f8543 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -235,9 +235,9 @@ 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_"; + // Prefix of the per-element scalar declarations RemapImageArrayElementUnits splits an + // image array into; the suffix is the array's own name and the element's index. + constexpr const char* IMAGE_ARRAY_ELEMENT_PREFIX = "mg_imageElem_"; // One image ARRAY whose elements the application pointed at units that are not // consecutive-from-element-zero. struct ImageArrayUnitPlan { @@ -253,33 +253,40 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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. + // array has no API side to fix, because ES makes glUniform1i on an image uniform an + // INVALID_OPERATION. // - // 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. + // Repaired by SPLITTING the array into one SCALAR image uniform per element, each with + // its own layout(binding = N), and rewriting `name[k]` to the scalar declared for + // element k. One declaration carries one binding, so one declaration per unit is the + // only spelling that reaches an arbitrary set of them. + // + // That rewrite needs every k in the emitted text to be a LITERAL, and it is: + // LegalizeResourceArrayIndexingForEssl has already folded or lowered every dynamic + // image-array subscript in the module, because ESSL forbids one outright ("image arrays + // indexed with non-constant expressions are forbidden in GLSL ES", Mesa 26.1.4 at + // ES 3.2, on a raw GLES probe with no MobileGL in the loop). The earlier shape here - + // widening the array to cover the whole span of units and routing each subscript through + // a `const highp int` offset table - was written before that pass covered images, and + // the table lookup was itself one of the non-constant expressions the same probe refuses. + // The split also costs exactly the image uniforms the application declared, where the + // widening cost the whole SPAN (seven for the four elements of + // KHR-GL42.shader_image_load_store.advanced-sso-simple), so there is no budget for it to + // fail to fit in. // // 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. + // caller to report - when the emitted extent disagrees with the reflection, when the + // array is reached by anything other than a subscript, or when a subscript is not a + // literal element index. 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. + // SplitReadWriteImageUniforms (so each element that is both read and written is split + // with its own binding already on it) 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); + Vector* outDeclined = nullptr); // The member list of a `gl_PerVertex { ... }` redeclaration in already-emitted ESSL - // the text between the braces, verbatim - or nullopt when the shader does not redeclare // the block in that direction. `input` selects the `in gl_PerVertex` form over the diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp index fcdb3cf0..7828a875 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ImageLoadStoreSsoScenario.cpp @@ -129,21 +129,20 @@ void main() // assignment that is not consecutive (the conformance case uses 0, 2, 4, 6) has no // spelling in a single declaration. // - // 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. + // RemapImageArrayElementUnits repairs it by SPLITTING the array into one scalar + // image uniform per element, each carrying its own binding, which costs exactly the + // four image uniforms the application declared. (It used to WIDEN the array to cover + // the whole span instead, which cost seven for those four elements and had to be + // declined on a stage that could not afford them - hence the budget gate that used + // to be here.) DirectVulkan needs no rewrite 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; + // One per element of the four-element array either fragment program declares. + return maxFragmentImageUniforms >= 4; } // The scenarios below need image load/store at all; a driver without it should skip @@ -174,8 +173,7 @@ void main() if (!Ready()) return; if (!ImagesAreUsable()) GTEST_SKIP() << "fewer than 8 image units"; if (!PerElementImageUnitsAreHonoured()) { - GTEST_SKIP() << "fewer than 7 fragment image uniforms: the widening that covers a " - "non-consecutive image array does not fit"; + GTEST_SKIP() << "fewer than 4 fragment image uniforms: the array under test does not fit"; } HeadlessGL& gl = Gl(); diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 7e4acfb1..d01298d0 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -21,7 +21,7 @@ using MobileGL::MG_Backend::DirectGLES::PrgramImpl::BuildPassthroughTessControlE using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ExtractPerVertexBlockMembers; 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_ARRAY_ELEMENT_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; @@ -52,7 +52,10 @@ 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; } + // The scalar RemapImageArrayElementUnits declares for one element of a split image array. + String Elem(const String& name, Int element) { + return String(IMAGE_ARRAY_ELEMENT_PREFIX) + name + "_" + std::to_string(element); + } } // namespace // The bug the pass exists for. SPIRV-Cross speculatively marks every storage image @@ -517,17 +520,18 @@ TEST(SplitReadWriteImageUniformsTest, EveryStageTagIsDistinct) { // 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). + // The advanced-sso-simple shape: a four-element image array on units 0, 2, 4, 6. The + // subscripts are literals because LegalizeResourceArrayIndexingForEssl has already folded + // the conformance case's `for (int i = 0; i < g_image.length(); ++i)` - ESSL forbids a + // non-constant image-array subscript outright, so a loop counter never reaches this pass. 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)); - } + imageStore(g_image[0], ivec2(gl_FragCoord.xy), vec4(1.0)); + imageStore(g_image[1], ivec2(gl_FragCoord.xy), vec4(1.0)); + imageStore(g_image[2], ivec2(gl_FragCoord.xy), vec4(1.0)); + imageStore(g_image[3], ivec2(gl_FragCoord.xy), vec4(1.0)); } )"; @@ -539,26 +543,34 @@ void main() } } // 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) { +// The defect, end to end. Elements 0..3 need units 0, 2, 4, 6, so the array becomes four scalars +// carrying those four bindings. Before this, the single stamped binding sent the four elements to +// units 0, 1, 2, 3. +TEST(RemapImageArrayElementUnitsTest, NonConsecutiveUnitsSplitIntoOneScalarPerElement) { Vector declined; const String out = - RemapImageArrayElementUnits(kSsoImageArrayFS, {Plan("g_image", {0, 2, 4, 6})}, 8, &declined); + RemapImageArrayElementUnits(kSsoImageArrayFS, {Plan("g_image", {0, 2, 4, 6})}, &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. + const Int units[4] = {0, 2, 4, 6}; + for (Int element = 0; element < 4; ++element) { + EXPECT_TRUE(Contains(out, "layout(rgba32f, binding = " + std::to_string(units[element]) + + ") uniform writeonly highp image2D " + Elem("g_image", element) + ";")) + << out; + EXPECT_TRUE(Contains(out, "imageStore(" + Elem("g_image", element) + ", ivec2(gl_FragCoord.xy)")) + << out; + } + // The array is gone entirely; nothing may still address units 0,1,2,3 through it. EXPECT_FALSE(Contains(out, "image2D g_image[4];")) << out; + EXPECT_FALSE(Contains(out, "g_image[")) << out; + // Exactly the four image uniforms the application declared - what the earlier widening cost + // was the whole SPAN, seven here, which is the budget failure mode this shape removes. + EXPECT_EQ(CountOf(out, "image2D "), 4u) << 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) { +// The other program of the same conformance case: units 1, 3, 5, 7 in the application's own +// element ORDER, which is what carries the assignment, so it must NOT be sorted or rebased. +TEST(RemapImageArrayElementUnitsTest, EachElementCarriesTheUnitTheApplicationGaveIt) { const String source = R"(#version 320 es layout(rgba32f, binding = 3) uniform writeonly highp image2D g_image[4]; void main() @@ -567,14 +579,17 @@ void main() 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; + const String out = RemapImageArrayElementUnits(source, {Plan("g_image", {3, 1, 7, 5})}); + const Int units[4] = {3, 1, 7, 5}; + for (Int element = 0; element < 4; ++element) { + EXPECT_TRUE(Contains(out, "binding = " + std::to_string(units[element]) + + ") uniform writeonly highp image2D " + Elem("g_image", element) + ";")) + << out; + } + // Only elements 0 and 3 are ever accessed; elements 1 and 2 are declared and unused, because + // the reflection says the array has four of them. + EXPECT_TRUE(Contains(out, "imageStore(" + Elem("g_image", 0) + ", ivec2(0)")) << out; + EXPECT_TRUE(Contains(out, "imageStore(" + Elem("g_image", 3) + ", ivec2(0)")) << out; } // Consecutive-from-element-zero is exactly what ESSL does unaided, so the emitted text of an @@ -588,33 +603,49 @@ void main() imageStore(g_image[1], ivec2(0), vec4(1.0)); } )"; - EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {2, 3, 4})}, 8), source); + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {2, 3, 4})}), 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); + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("other_image", {0, 4})}), 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) { +// A subscript that is not a literal names no element, so there is no scalar to rewrite it to. +// It should never arrive - LegalizeResourceArrayIndexingForEssl runs first and ESSL rejects the +// shape outright - but if one does, guessing an element would only change WHICH unit the access +// reaches wrongly. Decline, loudly, and change nothing. +TEST(RemapImageArrayElementUnitsTest, ANonLiteralSubscriptIsDeclinedAndNamed) { + const String source = 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)); + } +} +)"; Vector declined; - const String out = - RemapImageArrayElementUnits(kSsoImageArrayFS, {Plan("g_image", {0, 2, 4, 6})}, 4, &declined); - EXPECT_EQ(out, String(kSsoImageArrayFS)) << out; + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 2, 4, 6})}, &declined), source); 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 literal that is out of the reflected range is the same class of mismatch. + const String outOfRange = R"(#version 320 es +layout(rgba32f, binding = 0) uniform writeonly highp image2D g_image[2]; +void main() +{ + imageStore(g_image[5], ivec2(0), vec4(1.0)); +} +)"; + Vector outOfRangeDeclined; + EXPECT_EQ(RemapImageArrayElementUnits(outOfRange, {Plan("g_image", {0, 5})}, &outOfRangeDeclined), + outOfRange); + ASSERT_EQ(outOfRangeDeclined.size(), 1u); } -// 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. +// A use the pass cannot see a subscript on has no element index to rewrite, so splitting the +// array out from under it would leave it naming a declaration that no longer exists. 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]; @@ -626,7 +657,7 @@ void main() } )"; Vector declined; - EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 5})}, 8, &declined), source); + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 5})}, &declined), source); ASSERT_EQ(declined.size(), 1u); EXPECT_TRUE(Contains(declined[0], "g_image")) << declined[0]; } @@ -642,14 +673,15 @@ void main() } )"; Vector declined; - EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 4, 8})}, 16, &declined), source); + EXPECT_EQ(RemapImageArrayElementUnits(source, {Plan("g_image", {0, 4, 8})}, &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 +// The two passes that run after it have to see the split declarations and keep their bindings: 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) { +// is, and an element that is both read and written is split again into a pair that must BOTH +// carry that element's own unit. +TEST(RemapImageArrayElementUnitsTest, TheSplitElementsSurviveTheLaterImagePasses) { const String source = R"(#version 320 es layout(rgba32f, binding = 4) uniform highp image2D g_image[2]; void main() @@ -657,19 +689,44 @@ void main() imageStore(g_image[1], ivec2(0), imageLoad(g_image[0], ivec2(0))); } )"; - String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 6})}, 8); + String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 6})}); 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; + // Element 0 is only ever loaded and element 1 only ever stored, so neither is split into a + // pair - but each keeps the unit the application gave it, which the array could not express. + EXPECT_TRUE(Contains(out, "binding = 4")) << out; + EXPECT_TRUE(Contains(out, "binding = 6")) << out; + EXPECT_TRUE(Contains(out, "readonly highp image2D " + StageAlias(Elem("g_image", 0)) + ";")) << out; + EXPECT_TRUE(Contains(out, "writeonly highp image2D " + StageAlias(Elem("g_image", 1)) + ";")) << out; + EXPECT_TRUE(Contains(out, "imageStore(" + StageAlias(Elem("g_image", 1)) + ", ivec2(0), imageLoad(" + + StageAlias(Elem("g_image", 0)) + ", ivec2(0)))")) + << out; + // Nothing is left addressing the array. + EXPECT_FALSE(Contains(out, "g_image[")) << out; +} + +// The same element both read and written IS split into a coherent pair, and both halves have to +// inherit that element's binding - the shape the widening used to have to carry on an array. +TEST(RemapImageArrayElementUnitsTest, AnElementThatIsBothReadAndWrittenIsSplitWithItsOwnBinding) { + 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[1], ivec2(0))); + imageStore(g_image[0], ivec2(0), vec4(0.0)); +} +)"; + String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 9})}); + out = SplitReadWriteImageUniforms(out, kStage); + out = RemoveLayoutBinding(out); + + // Element 1 sits on unit 9, and both halves of its split pair say so. + EXPECT_EQ(CountOf(out, "binding = 9"), 2u) << out; + EXPECT_TRUE(Contains(out, "readonly highp image2D " + StageAlias(Elem("g_image", 1)) + ";")) << out; + EXPECT_TRUE(Contains(out, "writeonly highp image2D " + WriteAlias(StageAlias(Elem("g_image", 1))) + ";")) + << out; + EXPECT_EQ(CountOf(out, "binding = 4"), 1u) << out; } // --------------------------------------------------------------------------------------- diff --git a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt index ba0df960..8e92d409 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt +++ b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt @@ -12,7 +12,7 @@ add_executable( UniquifyIoBlockNamesTest.cpp LowerViewportIndexTest.cpp ClampMultisampleFetchTest.cpp - LegalizeStorageBlockArrayIndexTest.cpp + LegalizeResourceArrayIndexTest.cpp FlattenAtomicCounterBlockTest.cpp WidenImageFormatsTest.cpp ) diff --git a/MobileGL/MG_Test/ShaderTranspiler/LegalizeResourceArrayIndexTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/LegalizeResourceArrayIndexTest.cpp new file mode 100644 index 00000000..19cfb6c6 --- /dev/null +++ b/MobileGL/MG_Test/ShaderTranspiler/LegalizeResourceArrayIndexTest.cpp @@ -0,0 +1,505 @@ +// MobileGL - MobileGL/MG_Test/ShaderTranspiler/LegalizeResourceArrayIndexTest.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include + +#define SPV_ENABLE_UTILITY_CODE +#include "glslang/SPIRV/spirv.hpp11" +#undef SPV_ENABLE_UTILITY_CODE + +#include "Includes.h" +#include +#include +#include + +#include + +#include +#include + +using namespace MobileGL; +using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; + +namespace { + constexpr SizeT kSpirvHeaderWordCount = 5u; + + template + void ForEachInstruction(const Vector& spirv, Visitor&& visit) { + for (SizeT offset = kSpirvHeaderWordCount; offset < spirv.size();) { + const Uint32 wordCount = spirv[offset] >> 16u; + if (wordCount == 0u || offset + wordCount > spirv.size()) break; + visit(static_cast(spirv[offset] & 0xffffu), &spirv[offset], wordCount); + offset += wordCount; + } + } + + Vector CompileCompute(const String& source) { + using namespace MobileGL::MG_Util::ShaderTranspiler; + ShaderAttrib shaderAttrib{.shaderType = GL_COMPUTE_SHADER, .sourceStr = source}; + auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); + EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log); + if (!shaderResult) return {}; + + ProgramAttrib programAttrib{.shaders = {shaderResult.value()}}; + auto programResult = ShaderCompiler::LinkProgram(programAttrib); + EXPECT_TRUE(programResult) << (programResult ? String{} : programResult.error().log); + if (!programResult) return {}; + + ProgramBinaryAttrib binaryAttrib{.shaderTypes = {GL_COMPUTE_SHADER}, .program = *programResult.value()}; + auto binaryResult = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + EXPECT_TRUE(binaryResult) << (binaryResult ? String{} : binaryResult.error().log); + if (!binaryResult || binaryResult->empty()) return {}; + return binaryResult->front(); + } + + bool Validates(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + tools.SetMessageConsumer( + [](spv_message_level_t, const char*, const spv_position_t& position, const char* message) { + ADD_FAILURE() << "spirv-val at word " << position.index << ": " << message; + }); + return tools.Validate(spirv); + } + + Uint32 CountOpcode(const Vector& spirv, spv::Op wanted) { + Uint32 count = 0u; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32*, Uint32) { + if (opcode == wanted) ++count; + }); + return count; + } + + // Test-side reference walker, deliberately independent of the production detection so a + // bug in the pass cannot hide behind the same helper: true when some access chain rooted + // at an array-of-storage-blocks variable carries a non-constant FIRST index, which is + // exactly what the Qualcomm ES compiler refuses. + bool HasDynamicBlockArrayIndex(const Vector& spirv) { + std::set blockStructs; // OpTypeStruct ids decorated Block / BufferBlock + std::set constants; // OpConstant / OpConstantNull result ids + std::set blockArrayTypes; // OpTypeArray ids whose element is such a struct + std::set blockArrayPointers;// OpTypePointer ids pointing at one of those arrays + std::set blockArrayVars; // OpVariable ids of one of those pointer types + + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + switch (opcode) { + case spv::Op::OpDecorate: + if (wordCount >= 3u) { + const auto decoration = static_cast(words[2]); + if (decoration == spv::Decoration::Block || + decoration == spv::Decoration::BufferBlock) { + blockStructs.insert(words[1]); + } + } + break; + case spv::Op::OpConstant: + if (wordCount >= 3u) constants.insert(words[2]); + break; + case spv::Op::OpConstantNull: + if (wordCount >= 3u) constants.insert(words[2]); + break; + case spv::Op::OpTypeArray: + // OpTypeArray + if (wordCount >= 4u && blockStructs.count(words[2]) != 0u) { + blockArrayTypes.insert(words[1]); + } + break; + case spv::Op::OpTypePointer: + // OpTypePointer + if (wordCount >= 4u && blockArrayTypes.count(words[3]) != 0u) { + blockArrayPointers.insert(words[1]); + } + break; + case spv::Op::OpVariable: + // OpVariable + if (wordCount >= 4u && blockArrayPointers.count(words[1]) != 0u) { + blockArrayVars.insert(words[2]); + } + break; + default: + break; + } + }); + + bool dynamic = false; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpAccessChain && opcode != spv::Op::OpInBoundsAccessChain) return; + // OpAccessChain ... + if (wordCount < 5u) return; + if (blockArrayVars.count(words[3]) == 0u) return; + if (constants.count(words[4]) != 0u) return; + dynamic = true; + }); + return dynamic; + } + + // The image half of the same reference walker, and equally independent of the production + // detection: true when some access chain rooted at an array-of-IMAGES variable carries a + // non-constant FIRST index. A UniformConstant array whose element type is an OpTypeImage + // with Sampled == 2 is what GLSL spells `image2D g_image[N]`; a sampler array is an + // OpTypeSampledImage and is deliberately not matched here, because ESSL allows it a + // dynamically-uniform index. + bool HasDynamicImageArrayIndex(const Vector& spirv) { + std::set storageImages; // OpTypeImage ids with Sampled == 2 + std::set constants; // OpConstant / OpConstantNull result ids + std::set imageArrayTypes; // OpTypeArray ids whose element is such an image + std::set imageArrayPointers; // OpTypePointer ids pointing at one of those arrays + std::set imageArrayVars; // OpVariable ids of one of those pointer types + + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + switch (opcode) { + case spv::Op::OpTypeImage: + // OpTypeImage + if (wordCount >= 8u && words[7] == 2u) storageImages.insert(words[1]); + break; + case spv::Op::OpConstant: + case spv::Op::OpConstantNull: + if (wordCount >= 3u) constants.insert(words[2]); + break; + case spv::Op::OpTypeArray: + if (wordCount >= 4u && storageImages.count(words[2]) != 0u) { + imageArrayTypes.insert(words[1]); + } + break; + case spv::Op::OpTypePointer: + if (wordCount >= 4u && imageArrayTypes.count(words[3]) != 0u) { + imageArrayPointers.insert(words[1]); + } + break; + case spv::Op::OpVariable: + if (wordCount >= 4u && imageArrayPointers.count(words[1]) != 0u) { + imageArrayVars.insert(words[2]); + } + break; + default: + break; + } + }); + + bool dynamic = false; + ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { + if (opcode != spv::Op::OpAccessChain && opcode != spv::Op::OpInBoundsAccessChain) return; + if (wordCount < 5u) return; + if (imageArrayVars.count(words[3]) == 0u) return; + if (constants.count(words[4]) != 0u) return; + dynamic = true; + }); + return dynamic; + } + + // The ESSL SPIRV-Cross prints for a module, or the error it refused with. This is where the + // rule actually bites: the SPIR-V is legal Vulkan either way, and what a strict ES driver + // reads is this text. + struct EsslAttempt { + Bool succeeded = false; + String text; + String error; + }; + + EsslAttempt EmitEssl(const Vector& spirv) { + using namespace MobileGL::MG_Util::ShaderTranspiler; + EsslAttempt attempt; + SpvcSession session(spirv, SessionUsageBit::Transpile); + spvc_compiler_options options; + if (session.CreateOptions(&options) != SPVC_SUCCESS) return attempt; + spvc_compiler_options_set_uint(options, SPVC_COMPILER_OPTION_GLSL_VERSION, 320); + spvc_compiler_options_set_bool(options, SPVC_COMPILER_OPTION_GLSL_ES, SPVC_TRUE); + spvc_compiler_options_set_bool(options, SPVC_COMPILER_OPTION_GLSL_VULKAN_SEMANTICS, SPVC_FALSE); + if (session.SetOptions(options) != SPVC_SUCCESS) return attempt; + auto essl = ShaderCompiler::DecompileShader(session); + if (!essl) { + attempt.error = essl.error().log; + return attempt; + } + attempt.succeeded = true; + attempt.text = *essl; + return attempt; + } + + // `for (i = 0; i < 4; ++i)` over an array of storage blocks - the shape + // KHR-GL43.shader_storage_buffer_object.basic-stdLayout-case1 uses. Foldable: the + // induction variable is a literal after unrolling. + constexpr const char* kLoopIndexedBlockArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; +layout(std430, binding = 8) buffer Out { uint data[4]; } g_out; +void main() { + for (int i = 0; i < 4; ++i) { + g_out.data[i] = g_blocks[i].data[0]; + } +} +)"; + + // A uniform-sourced index - the shape + // KHR-GL43.shader_storage_buffer_object.advanced-indirectAddressing-case2 uses. Nothing + // can fold it, so the switch/select lowering is what has to carry it. + constexpr const char* kUniformIndexedBlockArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +uniform int g_index; +void main() { + g_blocks[g_index].data[0] = 7u; + g_out.value = g_blocks[g_index].data[1]; +} +)"; + + // The positive control from the device run: dynamic addressing through an array MEMBER of + // ONE block is legal ES and must not be rewritten. + constexpr const char* kArrayMemberInsideOneBlock = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_block; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +uniform int g_index; +void main() { + g_out.value = g_block.data[g_index]; +} +)"; + + // A block array indexed only with literals is already legal ES. + constexpr const char* kConstantIndexedBlockArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +void main() { + g_out.value = g_blocks[2].data[0] + g_blocks[3].data[1]; +} +)"; + + // The IMAGE half, and the case that has always been broken independently of any per-element + // unit remapping: a plain CONSECUTIVE image array subscripted by a loop variable. This is + // KHR-GL42.shader_image_load_store.advanced-sso-simple's own fragment shader shape, and a raw + // GLES probe on Mesa 26.1.4 at ES 3.2 refuses the ESSL it produces with "image arrays indexed + // with non-constant expressions are forbidden in GLSL ES". Foldable: after unrolling every + // subscript is a literal. + constexpr const char* kLoopIndexedImageArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(rgba32f, binding = 0) uniform writeonly image2D g_image[4]; +void main() { + for (int i = 0; i < 4; ++i) { + imageStore(g_image[i], ivec2(0), vec4(1.0)); + } +} +)"; + + // A uniform-sourced image index: nothing can fold it, so the switch/select lowering is what + // has to carry it. Both directions in one shader, as the block-array fixture does. + constexpr const char* kUniformIndexedImageArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(rgba32f, binding = 0) uniform image2D g_image[4]; +layout(std430, binding = 8) buffer Out { vec4 value; } g_out; +uniform int g_index; +void main() { + imageStore(g_image[g_index], ivec2(0), vec4(7.0)); + g_out.value = imageLoad(g_image[g_index], ivec2(1)); +} +)"; + + // An image array indexed only with literals is already legal ES. + constexpr const char* kConstantIndexedImageArray = R"(#version 450 core +layout(local_size_x = 1) in; +layout(rgba32f, binding = 0) uniform writeonly image2D g_image[4]; +void main() { + imageStore(g_image[1], ivec2(0), vec4(1.0)); + imageStore(g_image[3], ivec2(0), vec4(2.0)); +} +)"; + + // The positive control for the scope decision: ESSL 3.20 4.1.7 allows a SAMPLER array a + // dynamically-uniform index, and the same raw GLES probe confirms it - both a loop-variable + // subscript and a const-table lookup compile and link. Nothing here may be rewritten. + constexpr const char* kUniformIndexedSamplerArray = R"(#version 450 core +layout(local_size_x = 1) in; +uniform sampler2D g_tex[4]; +layout(std430, binding = 8) buffer Out { vec4 value; } g_out; +uniform int g_index; +void main() { + g_out.value = texture(g_tex[g_index], vec2(0.5)); +} +)"; + + // An imageAtomic* reaches the array through OpImageTexelPointer, and running one per element + // would perform every other element's atomic as well. The pass has to decline rather than + // lower this. + constexpr const char* kUniformIndexedImageAtomic = R"(#version 450 core +layout(local_size_x = 1) in; +layout(r32ui, binding = 0) uniform uimage2D g_image[4]; +layout(std430, binding = 8) buffer Out { uint value; } g_out; +uniform int g_index; +void main() { + g_out.value = imageAtomicAdd(g_image[g_index], ivec2(0), 1u); +} +)"; +} // namespace + +TEST(LegalizeResourceArrayIndexPass, FoldsALoopIndexedBlockArray) { + const Vector input = CompileCompute(kLoopIndexedBlockArray); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicBlockArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + // Either half of the legalization is an acceptable outcome here - what the ES driver + // cares about is only that no dynamic subscript survives. + EXPECT_FALSE(HasDynamicBlockArrayIndex(output)); + EXPECT_TRUE(Validates(output)); +} + +TEST(LegalizeResourceArrayIndexPass, LowersAUniformIndexedWriteToASwitchAndAReadToSelects) { + const Vector input = CompileCompute(kUniformIndexedBlockArray); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicBlockArrayIndex(input)); + EXPECT_EQ(CountOpcode(input, spv::Op::OpSwitch), 0u); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_FALSE(HasDynamicBlockArrayIndex(output)); + // One switch for the store, and one select per element past the first for the load. + EXPECT_EQ(CountOpcode(output, spv::Op::OpSwitch), 1u); + EXPECT_EQ(CountOpcode(output, spv::Op::OpSelect), 3u); + EXPECT_TRUE(Validates(output)); +} + +TEST(LegalizeResourceArrayIndexPass, LeavesADynamicMemberOfOneBlockByteIdentical) { + const Vector input = CompileCompute(kArrayMemberInsideOneBlock); + ASSERT_FALSE(input.empty()); + EXPECT_FALSE(HasDynamicBlockArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +TEST(LegalizeResourceArrayIndexPass, LeavesAConstantIndexedBlockArrayByteIdentical) { + const Vector input = CompileCompute(kConstantIndexedBlockArray); + ASSERT_FALSE(input.empty()); + EXPECT_FALSE(HasDynamicBlockArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +TEST(LegalizeResourceArrayIndexPass, IsIdempotent) { + const Vector input = CompileCompute(kUniformIndexedBlockArray); + ASSERT_FALSE(input.empty()); + + Vector once; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, once, true)); + ASSERT_FALSE(once.empty()); + + Vector twice; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(once, twice, true)); + EXPECT_EQ(twice, once); +} + +// The case no test covered before, and the one that has nothing to do with per-element unit +// remapping: an ordinary consecutive image array written from a loop. Every emitted subscript has +// to end up a literal, or the ES driver drops the stage and every draw with it. +TEST(LegalizeResourceArrayIndexPass, FoldsALoopIndexedImageArray) { + const Vector input = CompileCompute(kLoopIndexedImageArray); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicImageArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_FALSE(HasDynamicImageArrayIndex(output)); + EXPECT_TRUE(Validates(output)); + + // ...and in the text the driver actually reads. Before: `g_image[i]`; after: four literals. + const EsslAttempt before = EmitEssl(input); + ASSERT_TRUE(before.succeeded) << before.error; + EXPECT_NE(before.text.find("g_image[i]"), String::npos) << before.text; + + const EsslAttempt after = EmitEssl(output); + ASSERT_TRUE(after.succeeded) << after.error; + EXPECT_EQ(after.text.find("g_image[i]"), String::npos) << after.text; + for (int element = 0; element < 4; ++element) { + EXPECT_NE(after.text.find("g_image[" + std::to_string(element) + "]"), String::npos) << after.text; + } +} + +TEST(LegalizeResourceArrayIndexPass, LowersAUniformIndexedImageWriteToASwitchAndAReadToSelects) { + const Vector input = CompileCompute(kUniformIndexedImageArray); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicImageArrayIndex(input)); + EXPECT_EQ(CountOpcode(input, spv::Op::OpSwitch), 0u); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_FALSE(HasDynamicImageArrayIndex(output)); + // One switch for the imageStore, and one select per element past the first for the imageLoad. + // The selection is on the loaded TEXEL, never on the image object - an opaque type cannot be + // selected at all - so there is one OpImageRead per element behind those selects. + EXPECT_EQ(CountOpcode(output, spv::Op::OpSwitch), 1u); + EXPECT_EQ(CountOpcode(output, spv::Op::OpSelect), 3u); + EXPECT_EQ(CountOpcode(output, spv::Op::OpImageRead), 4u); + EXPECT_EQ(CountOpcode(output, spv::Op::OpImageWrite), 4u); + EXPECT_TRUE(Validates(output)); + + const EsslAttempt after = EmitEssl(output); + ASSERT_TRUE(after.succeeded) << after.error; + for (int element = 0; element < 4; ++element) { + EXPECT_NE(after.text.find("g_image[" + std::to_string(element) + "]"), String::npos) << after.text; + } +} + +TEST(LegalizeResourceArrayIndexPass, LeavesAConstantIndexedImageArrayByteIdentical) { + const Vector input = CompileCompute(kConstantIndexedImageArray); + ASSERT_FALSE(input.empty()); + EXPECT_FALSE(HasDynamicImageArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +// The scope decision, asserted rather than assumed: a sampler array indexed by a uniform is legal +// ESSL, so the module must come back untouched - not merely legal, byte for byte the same. +TEST(LegalizeResourceArrayIndexPass, LeavesADynamicallyIndexedSamplerArrayByteIdentical) { + const Vector input = CompileCompute(kUniformIndexedSamplerArray); + ASSERT_FALSE(input.empty()); + EXPECT_FALSE(HasDynamicImageArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + EXPECT_EQ(output, input); +} + +// An imageAtomic* is the shape the lowering must refuse: its per-element rebuild would run every +// other element's read-modify-write. Declining leaves the illegal subscript in place - which is +// what the latched warning in LegalizeResourceArrayIndexingForEssl is for - but a half-transform +// would corrupt four images instead of losing one stage. +TEST(LegalizeResourceArrayIndexPass, DeclinesAUniformIndexedImageAtomic) { + const Vector input = CompileCompute(kUniformIndexedImageAtomic); + ASSERT_FALSE(input.empty()); + EXPECT_TRUE(HasDynamicImageArrayIndex(input)); + + Vector output; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_TRUE(HasDynamicImageArrayIndex(output)); + EXPECT_EQ(CountOpcode(output, spv::Op::OpSwitch), 0u); + EXPECT_TRUE(Validates(output)); +} + +TEST(LegalizeResourceArrayIndexPass, IsIdempotentOnImageArrays) { + const Vector input = CompileCompute(kUniformIndexedImageArray); + ASSERT_FALSE(input.empty()); + + Vector once; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(input, once, true)); + ASSERT_FALSE(once.empty()); + + Vector twice; + ASSERT_TRUE(ShaderCompiler::LegalizeResourceArrayIndexingForEssl(once, twice, true)); + EXPECT_EQ(twice, once); +} diff --git a/MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp deleted file mode 100644 index f4c7bfbd..00000000 --- a/MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp +++ /dev/null @@ -1,251 +0,0 @@ -// MobileGL - MobileGL/MG_Test/ShaderTranspiler/LegalizeStorageBlockArrayIndexTest.cpp -// Copyright (c) 2026 MobileGL-Dev -// Licensed under the GNU Lesser General Public License v3.0: -// https://www.gnu.org/licenses/gpl-3.0.txt -// https://www.gnu.org/licenses/lgpl-3.0.txt -// SPDX-License-Identifier: LGPL-3.0-only -// End of Source File Header - -#include - -#define SPV_ENABLE_UTILITY_CODE -#include "glslang/SPIRV/spirv.hpp11" -#undef SPV_ENABLE_UTILITY_CODE - -#include "Includes.h" -#include -#include - -#include - -#include -#include - -using namespace MobileGL; -using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; - -namespace { - constexpr SizeT kSpirvHeaderWordCount = 5u; - - template - void ForEachInstruction(const Vector& spirv, Visitor&& visit) { - for (SizeT offset = kSpirvHeaderWordCount; offset < spirv.size();) { - const Uint32 wordCount = spirv[offset] >> 16u; - if (wordCount == 0u || offset + wordCount > spirv.size()) break; - visit(static_cast(spirv[offset] & 0xffffu), &spirv[offset], wordCount); - offset += wordCount; - } - } - - Vector CompileCompute(const String& source) { - using namespace MobileGL::MG_Util::ShaderTranspiler; - ShaderAttrib shaderAttrib{.shaderType = GL_COMPUTE_SHADER, .sourceStr = source}; - auto shaderResult = ShaderCompiler::CompileShader(shaderAttrib); - EXPECT_TRUE(shaderResult) << (shaderResult ? String{} : shaderResult.error().log); - if (!shaderResult) return {}; - - ProgramAttrib programAttrib{.shaders = {shaderResult.value()}}; - auto programResult = ShaderCompiler::LinkProgram(programAttrib); - EXPECT_TRUE(programResult) << (programResult ? String{} : programResult.error().log); - if (!programResult) return {}; - - ProgramBinaryAttrib binaryAttrib{.shaderTypes = {GL_COMPUTE_SHADER}, .program = *programResult.value()}; - auto binaryResult = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); - EXPECT_TRUE(binaryResult) << (binaryResult ? String{} : binaryResult.error().log); - if (!binaryResult || binaryResult->empty()) return {}; - return binaryResult->front(); - } - - bool Validates(const Vector& spirv) { - spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); - tools.SetMessageConsumer( - [](spv_message_level_t, const char*, const spv_position_t& position, const char* message) { - ADD_FAILURE() << "spirv-val at word " << position.index << ": " << message; - }); - return tools.Validate(spirv); - } - - Uint32 CountOpcode(const Vector& spirv, spv::Op wanted) { - Uint32 count = 0u; - ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32*, Uint32) { - if (opcode == wanted) ++count; - }); - return count; - } - - // Test-side reference walker, deliberately independent of the production detection so a - // bug in the pass cannot hide behind the same helper: true when some access chain rooted - // at an array-of-storage-blocks variable carries a non-constant FIRST index, which is - // exactly what the Qualcomm ES compiler refuses. - bool HasDynamicBlockArrayIndex(const Vector& spirv) { - std::set blockStructs; // OpTypeStruct ids decorated Block / BufferBlock - std::set constants; // OpConstant / OpConstantNull result ids - std::set blockArrayTypes; // OpTypeArray ids whose element is such a struct - std::set blockArrayPointers;// OpTypePointer ids pointing at one of those arrays - std::set blockArrayVars; // OpVariable ids of one of those pointer types - - ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { - switch (opcode) { - case spv::Op::OpDecorate: - if (wordCount >= 3u) { - const auto decoration = static_cast(words[2]); - if (decoration == spv::Decoration::Block || - decoration == spv::Decoration::BufferBlock) { - blockStructs.insert(words[1]); - } - } - break; - case spv::Op::OpConstant: - if (wordCount >= 3u) constants.insert(words[2]); - break; - case spv::Op::OpConstantNull: - if (wordCount >= 3u) constants.insert(words[2]); - break; - case spv::Op::OpTypeArray: - // OpTypeArray - if (wordCount >= 4u && blockStructs.count(words[2]) != 0u) { - blockArrayTypes.insert(words[1]); - } - break; - case spv::Op::OpTypePointer: - // OpTypePointer - if (wordCount >= 4u && blockArrayTypes.count(words[3]) != 0u) { - blockArrayPointers.insert(words[1]); - } - break; - case spv::Op::OpVariable: - // OpVariable - if (wordCount >= 4u && blockArrayPointers.count(words[1]) != 0u) { - blockArrayVars.insert(words[2]); - } - break; - default: - break; - } - }); - - bool dynamic = false; - ForEachInstruction(spirv, [&](spv::Op opcode, const Uint32* words, Uint32 wordCount) { - if (opcode != spv::Op::OpAccessChain && opcode != spv::Op::OpInBoundsAccessChain) return; - // OpAccessChain ... - if (wordCount < 5u) return; - if (blockArrayVars.count(words[3]) == 0u) return; - if (constants.count(words[4]) != 0u) return; - dynamic = true; - }); - return dynamic; - } - - // `for (i = 0; i < 4; ++i)` over an array of storage blocks - the shape - // KHR-GL43.shader_storage_buffer_object.basic-stdLayout-case1 uses. Foldable: the - // induction variable is a literal after unrolling. - constexpr const char* kLoopIndexedBlockArray = R"(#version 450 core -layout(local_size_x = 1) in; -layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; -layout(std430, binding = 8) buffer Out { uint data[4]; } g_out; -void main() { - for (int i = 0; i < 4; ++i) { - g_out.data[i] = g_blocks[i].data[0]; - } -} -)"; - - // A uniform-sourced index - the shape - // KHR-GL43.shader_storage_buffer_object.advanced-indirectAddressing-case2 uses. Nothing - // can fold it, so the switch/select lowering is what has to carry it. - constexpr const char* kUniformIndexedBlockArray = R"(#version 450 core -layout(local_size_x = 1) in; -layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; -layout(std430, binding = 8) buffer Out { uint value; } g_out; -uniform int g_index; -void main() { - g_blocks[g_index].data[0] = 7u; - g_out.value = g_blocks[g_index].data[1]; -} -)"; - - // The positive control from the device run: dynamic addressing through an array MEMBER of - // ONE block is legal ES and must not be rewritten. - constexpr const char* kArrayMemberInsideOneBlock = R"(#version 450 core -layout(local_size_x = 1) in; -layout(std430, binding = 0) buffer Blk { uint data[4]; } g_block; -layout(std430, binding = 8) buffer Out { uint value; } g_out; -uniform int g_index; -void main() { - g_out.value = g_block.data[g_index]; -} -)"; - - // A block array indexed only with literals is already legal ES. - constexpr const char* kConstantIndexedBlockArray = R"(#version 450 core -layout(local_size_x = 1) in; -layout(std430, binding = 0) buffer Blk { uint data[4]; } g_blocks[4]; -layout(std430, binding = 8) buffer Out { uint value; } g_out; -void main() { - g_out.value = g_blocks[2].data[0] + g_blocks[3].data[1]; -} -)"; -} // namespace - -TEST(LegalizeStorageBlockArrayIndexPass, FoldsALoopIndexedBlockArray) { - const Vector input = CompileCompute(kLoopIndexedBlockArray); - ASSERT_FALSE(input.empty()); - EXPECT_TRUE(HasDynamicBlockArrayIndex(input)); - - Vector output; - ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); - ASSERT_FALSE(output.empty()); - // Either half of the legalization is an acceptable outcome here - what the ES driver - // cares about is only that no dynamic subscript survives. - EXPECT_FALSE(HasDynamicBlockArrayIndex(output)); - EXPECT_TRUE(Validates(output)); -} - -TEST(LegalizeStorageBlockArrayIndexPass, LowersAUniformIndexedWriteToASwitchAndAReadToSelects) { - const Vector input = CompileCompute(kUniformIndexedBlockArray); - ASSERT_FALSE(input.empty()); - EXPECT_TRUE(HasDynamicBlockArrayIndex(input)); - EXPECT_EQ(CountOpcode(input, spv::Op::OpSwitch), 0u); - - Vector output; - ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); - ASSERT_FALSE(output.empty()); - EXPECT_FALSE(HasDynamicBlockArrayIndex(output)); - // One switch for the store, and one select per element past the first for the load. - EXPECT_EQ(CountOpcode(output, spv::Op::OpSwitch), 1u); - EXPECT_EQ(CountOpcode(output, spv::Op::OpSelect), 3u); - EXPECT_TRUE(Validates(output)); -} - -TEST(LegalizeStorageBlockArrayIndexPass, LeavesADynamicMemberOfOneBlockByteIdentical) { - const Vector input = CompileCompute(kArrayMemberInsideOneBlock); - ASSERT_FALSE(input.empty()); - EXPECT_FALSE(HasDynamicBlockArrayIndex(input)); - - Vector output; - ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); - EXPECT_EQ(output, input); -} - -TEST(LegalizeStorageBlockArrayIndexPass, LeavesAConstantIndexedBlockArrayByteIdentical) { - const Vector input = CompileCompute(kConstantIndexedBlockArray); - ASSERT_FALSE(input.empty()); - EXPECT_FALSE(HasDynamicBlockArrayIndex(input)); - - Vector output; - ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, output, true)); - EXPECT_EQ(output, input); -} - -TEST(LegalizeStorageBlockArrayIndexPass, IsIdempotent) { - const Vector input = CompileCompute(kUniformIndexedBlockArray); - ASSERT_FALSE(input.empty()); - - Vector once; - ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(input, once, true)); - ASSERT_FALSE(once.empty()); - - Vector twice; - ASSERT_TRUE(ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl(once, twice, true)); - EXPECT_EQ(twice, once); -} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index df9301ee..25555ee4 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -43,7 +43,7 @@ #include "SpirvPasses/StripNoPerspectivePass.h" #include "SpirvPasses/EmulateNoPerspectivePass.h" #include "SpirvPasses/LegalizeFragmentOutputIndexPass.h" -#include "SpirvPasses/LegalizeStorageBlockArrayIndexPass.h" +#include "SpirvPasses/LegalizeResourceArrayIndexPass.h" #include "SpirvPasses/FlattenAtomicCounterBlockPass.h" #include "spirv-tools/libspirv.h" #include "spirv-tools/optimizer.hpp" @@ -1015,16 +1015,16 @@ namespace MobileGL { return true; } - bool ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl( + bool ShaderCompiler::LegalizeResourceArrayIndexingForEssl( const Vector& inputBinary, Vector& outputBinary, const bool enableSpirvValidation) { using namespace spvtools; - // Detection gates everything: a module that declares no array of storage - // blocks, or indexes one only with constants - every shader but a handful - - // pays one BuildModule and is handed back byte for byte, so the folding chain - // can never perturb a shader that did not need it. - if (!LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + // Detection gates everything: a module that declares no array of storage blocks + // and no array of images, or indexes one only with constants - every shader but + // a handful - pays one BuildModule and is handed back byte for byte, so the + // folding chain can never perturb a shader that did not need it. + if (!LegalizeResourceArrayIndexPass::BinaryHasDynamicResourceArrayIndexing( inputBinary)) { outputBinary = inputBinary; return true; @@ -1040,7 +1040,7 @@ namespace MobileGL { // induction variable as an OpPhi, and glslang emits it as loads and stores of // a Function variable. folder.RegisterPass(CreateLocalMultiStoreElimPass()); - folder.RegisterPass(LegalizeStorageBlockArrayIndexPass::CreateMarkLoopsForUnrollPass()); + folder.RegisterPass(LegalizeResourceArrayIndexPass::CreateMarkLoopsForUnrollPass()); folder.RegisterPass(CreateLoopUnrollPass(true)); // Fold the unrolled induction values into the access chains, then clear out // what constant conditions leave behind. @@ -1050,14 +1050,14 @@ namespace MobileGL { folder.RegisterPass(CreateBlockMergePass()); Vector folded; - if (!RunOptimizerChecked("LegalizeStorageBlockArrayIndexingForEssl.fold", folder, + if (!RunOptimizerChecked("LegalizeResourceArrayIndexingForEssl.fold", folder, inputBinary, folded, true, enableSpirvValidation) || folded.empty()) { // Fail open onto the fallback rather than onto the illegal module. folded = inputBinary; } - if (!LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + if (!LegalizeResourceArrayIndexPass::BinaryHasDynamicResourceArrayIndexing( folded)) { outputBinary = folded; return true; @@ -1066,25 +1066,25 @@ namespace MobileGL { // Genuinely dynamic (uniform-derived, non-constant trip count, ...): lower it. Optimizer lowerer(SPV_ENV_VULKAN_1_1); lowerer.RegisterPass( - LegalizeStorageBlockArrayIndexPass::CreateLowerToConstantSwitchPass()); + LegalizeResourceArrayIndexPass::CreateLowerToConstantSwitchPass()); // The chains the lowering replaced are dead now; remove_outputs must stay // false here for the same reason it does in SanitizeAndOptimizeBinary. lowerer.RegisterPass(CreateAggressiveDCEPass(false)); - if (!RunOptimizerChecked("LegalizeStorageBlockArrayIndexingForEssl.lower", lowerer, folded, + if (!RunOptimizerChecked("LegalizeResourceArrayIndexingForEssl.lower", lowerer, folded, outputBinary, true, enableSpirvValidation) || outputBinary.empty()) { outputBinary = folded; return true; } - if (LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + if (LegalizeResourceArrayIndexPass::BinaryHasDynamicResourceArrayIndexing( outputBinary)) { // MGLOG_W, latched, for the same reason the fragment-output one is: this // runs per shader compile and shader packs compile lazily mid-session. - MGLOG_W_ONCE("[spirv] LegalizeStorageBlockArrayIndexingForEssl: an array of storage " - "blocks is still indexed dynamically; a strict ES driver will reject " - "this shader"); + MGLOG_W_ONCE("[spirv] LegalizeResourceArrayIndexingForEssl: an array of storage " + "blocks or of images is still indexed dynamically; a strict ES " + "driver will reject this shader"); } return true; } diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 70664175..41840591 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -161,18 +161,22 @@ namespace MobileGL { static bool LegalizeFragmentOutputIndexingForEssl(const Vector& inputBinary, Vector& outputBinary, bool enableSpirvValidation = false); - // Makes every index into an ARRAY OF SHADER STORAGE BLOCKS a constant integral - // expression. GL 4.3 allows any dynamically-uniform index there; the Qualcomm - // ES compiler enforces the ES 3.1 constant-expression rule and refuses the whole - // stage ("indexing into an SSBO array using a non-constant expression is not - // permitted"), which loses the program while the frontend still reports - // GL_LINK_STATUS = TRUE. Same two halves as the fragment-output legalization: - // fold the loop-derived indices, then lower whatever is genuinely dynamic to a - // switch over the array's range. DirectGLES transpile path only - Vulkan has no - // such restriction and must keep seeing one descriptor array. Copies the input - // through untouched when no block array is indexed dynamically, which is every - // shader but a handful. See LegalizeStorageBlockArrayIndexPass. - static bool LegalizeStorageBlockArrayIndexingForEssl(const Vector& inputBinary, + // Makes every index into an ARRAY OF SHADER STORAGE BLOCKS or an ARRAY OF IMAGE + // UNIFORMS a constant integral expression. Desktop GL allows any + // dynamically-uniform index in either; ES keeps the ES 3.1 + // constant-expression rule for both and the drivers refuse the whole stage + // ("indexing into an SSBO array using a non-constant expression is not + // permitted" on Qualcomm, "image arrays indexed with non-constant expressions + // are forbidden in GLSL ES" on Mesa), which loses the program while the + // frontend still reports GL_LINK_STATUS = TRUE. Same two halves as the + // fragment-output legalization: fold the loop-derived indices, then lower + // whatever is genuinely dynamic to a switch over the array's range. SAMPLER + // arrays are out of scope - ESSL 3.20 4.1.7 permits them a dynamically-uniform + // index. DirectGLES transpile path only - Vulkan has no such restriction and + // must keep seeing one descriptor array. Copies the input through untouched + // when no such array is indexed dynamically, which is every shader but a + // handful. See LegalizeResourceArrayIndexPass. + static bool LegalizeResourceArrayIndexingForEssl(const Vector& inputBinary, Vector& outputBinary, bool enableSpirvValidation = false); // Collapses each synthesized gl_AtomicCounterBlock_ into one uint array at diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.cpp similarity index 57% rename from MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp rename to MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.cpp index afa25909..4d729fd6 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.cpp @@ -1,4 +1,4 @@ -// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.cpp +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.cpp // Copyright (c) 2025-2026 MobileGL-Dev // Licensed under the GNU Lesser General Public License v3.0: // https://www.gnu.org/licenses/gpl-3.0.txt @@ -6,7 +6,7 @@ // SPDX-License-Identifier: LGPL-3.0-only // End of Source File Header -#include "LegalizeStorageBlockArrayIndexPass.h" +#include "LegalizeResourceArrayIndexPass.h" #include "spirv.hpp" #include "source/opt/basic_block.h" @@ -39,10 +39,10 @@ namespace MobileGL { using spvtools::opt::IRContext; using spvtools::opt::Operand; - // GL_MAX_*_SHADER_STORAGE_BLOCKS is 16 on the devices MobileGL targets, and - // each lowered element costs one basic block per write, so a module claiming - // more than this is refused rather than exploded. The largest array in the - // conformance suite is 8. + // GL_MAX_*_SHADER_STORAGE_BLOCKS and GL_MAX_*_IMAGE_UNIFORMS are both 16 or + // fewer on the devices MobileGL targets, and each lowered element costs one + // basic block per write, so a module claiming more than this is refused rather + // than exploded. The largest array in the conformance suite is 8. constexpr uint32_t kMaxLoweredArrayLength = 32; // One CFG-changing rewrite per round (analyses are dropped after each), so // the round budget bounds the work on a pathological module. @@ -50,14 +50,22 @@ namespace MobileGL { // Full unrolling copies the body once per iteration, and nothing in the stock // unroller bounds that. Past this count the loop is left alone and the switch // lowering, whose cost is the array length rather than the trip count, takes - // it instead. A loop over an array of storage blocks iterates at most - // GL_MAX_*_SHADER_STORAGE_BLOCKS times in any shader that is not already - // broken. + // it instead. A loop over an array of storage blocks or of images iterates at + // most GL_MAX_*_SHADER_STORAGE_BLOCKS / GL_MAX_*_IMAGE_UNIFORMS times in any + // shader that is not already broken. constexpr size_t kMaxUnrolledIterations = 64; + struct ResourceArray { + uint32_t length = 0; + // Which lowering the chain's uses need; see the header. Detection and + // loop-marking are identical for both. + bool isImage = false; + }; + struct DynamicIndexUse { Instruction* accessChain = nullptr; uint32_t arrayLength = 0; + bool isImageArray = false; }; bool HasDecoration(IRContext* context, uint32_t id, spv::Decoration kind) { @@ -73,16 +81,24 @@ namespace MobileGL { return false; } - // Every variable that is an ARRAY OF STORAGE BLOCKS, mapped to that array's - // length. Two spellings are accepted because both reach here depending on the - // SPIR-V version glslang targets: StorageBuffer + Block (1.3, what MobileGL - // asks for) and Uniform + BufferBlock (the pre-1.3 encoding). A UNIFORM block - // array - Uniform + Block - is deliberately NOT collected; see the header. + // Every variable that is an ARRAY OF STORAGE BLOCKS or an ARRAY OF IMAGE + // UNIFORMS, mapped to that array's length and kind. + // + // Storage blocks: two spellings are accepted because both reach here depending + // on the SPIR-V version glslang targets: StorageBuffer + Block (1.3, what + // MobileGL asks for) and Uniform + BufferBlock (the pre-1.3 encoding). A UNIFORM + // block array - Uniform + Block - is deliberately NOT collected; see the header. + // + // Images: UniformConstant + OpTypeArray of OpTypeImage. Sampled == 2 is what + // separates a storage image - what GLSL calls `image2D` and what the ES rule is + // about - from the OpTypeImage that sits INSIDE an OpTypeSampledImage, which + // never appears as an array element type on its own here and whose array ESSL + // 3.20 4.1.7 explicitly permits a dynamically-uniform index. // // A length that is not a plain OpConstant (a spec constant) maps to 0: still // detected as illegal ESSL, never lowered. - std::unordered_map CollectStorageBlockArrays(IRContext* context) { - std::unordered_map blockArrays; + std::unordered_map CollectResourceArrays(IRContext* context) { + std::unordered_map resourceArrays; auto* defUseMgr = context->get_def_use_mgr(); auto* constantMgr = context->get_constant_mgr(); @@ -93,7 +109,8 @@ namespace MobileGL { const auto storageClass = static_cast(inst.GetSingleWordInOperand(0)); if (storageClass != spv::StorageClass::StorageBuffer && - storageClass != spv::StorageClass::Uniform) { + storageClass != spv::StorageClass::Uniform && + storageClass != spv::StorageClass::UniformConstant) { continue; } @@ -106,17 +123,33 @@ namespace MobileGL { continue; } Instruction* elementType = defUseMgr->GetDef(pointeeType->GetSingleWordInOperand(0)); - if (elementType == nullptr || elementType->opcode() != spv::Op::OpTypeStruct) { + if (elementType == nullptr) { continue; } - const bool isStorageBlock = - storageClass == spv::StorageClass::StorageBuffer - ? HasDecoration(context, elementType->result_id(), spv::Decoration::Block) - : HasDecoration(context, elementType->result_id(), - spv::Decoration::BufferBlock); - if (!isStorageBlock) { - continue; + bool isImage = false; + if (storageClass == spv::StorageClass::UniformConstant) { + // OpTypeImage + // + if (elementType->opcode() != spv::Op::OpTypeImage || + elementType->NumInOperands() < 6 || + elementType->GetSingleWordInOperand(5) != 2u) { + continue; + } + isImage = true; + } else { + if (elementType->opcode() != spv::Op::OpTypeStruct) { + continue; + } + const bool isStorageBlock = + storageClass == spv::StorageClass::StorageBuffer + ? HasDecoration(context, elementType->result_id(), + spv::Decoration::Block) + : HasDecoration(context, elementType->result_id(), + spv::Decoration::BufferBlock); + if (!isStorageBlock) { + continue; + } } uint32_t arrayLength = 0; @@ -125,9 +158,9 @@ namespace MobileGL { if (lengthConstant != nullptr && lengthConstant->AsIntConstant() != nullptr) { arrayLength = lengthConstant->AsIntConstant()->GetU32BitValue(); } - blockArrays.emplace(inst.result_id(), arrayLength); + resourceArrays.emplace(inst.result_id(), ResourceArray{arrayLength, isImage}); } - return blockArrays; + return resourceArrays; } // "Constant integral expression" in the ESSL sense: an OpConstant (or the @@ -140,17 +173,17 @@ namespace MobileGL { def->opcode() == spv::Op::OpConstantNull); } - // Access chains that index an array of storage blocks with a non-constant. - // Only the FIRST index is considered: it is the one that selects the block, - // and it is the only one ESSL constrains here. Indices inside the block - the - // member selector and any array subscript below it - are legal however they - // are computed, and chains rooted at another access chain are already inside - // one element. + // Access chains that index an array of storage blocks or of images with a + // non-constant. Only the FIRST index is considered: it is the one that selects + // the element, and it is the only one ESSL constrains here. Indices inside the + // block - the member selector and any array subscript below it - are legal + // however they are computed, and chains rooted at another access chain are + // already inside one element. std::vector CollectDynamicIndexUses(IRContext* context) { std::vector uses; - const std::unordered_map blockArrays = - CollectStorageBlockArrays(context); - if (blockArrays.empty()) { + const std::unordered_map resourceArrays = + CollectResourceArrays(context); + if (resourceArrays.empty()) { return uses; } @@ -164,14 +197,15 @@ namespace MobileGL { if (inst.NumInOperands() < 2) { continue; } - const auto arrayIt = blockArrays.find(inst.GetSingleWordInOperand(0)); - if (arrayIt == blockArrays.end()) { + const auto arrayIt = resourceArrays.find(inst.GetSingleWordInOperand(0)); + if (arrayIt == resourceArrays.end()) { continue; } if (IsConstantIndex(context, inst.GetSingleWordInOperand(1))) { continue; } - uses.push_back({&inst, arrayIt->second}); + uses.push_back( + {&inst, arrayIt->second.length, arrayIt->second.isImage}); } } } @@ -274,7 +308,7 @@ namespace MobileGL { } } // namespace - bool LegalizeStorageBlockArrayIndexPass::BinaryHasDynamicStorageBlockArrayIndexing( + bool LegalizeResourceArrayIndexPass::BinaryHasDynamicResourceArrayIndexing( const std::vector& binary) { if (binary.empty()) { return false; @@ -289,11 +323,11 @@ namespace MobileGL { return !CollectDynamicIndexUses(context.get()).empty(); } - spvtools::opt::Pass::Status LegalizeStorageBlockArrayIndexPass::Process() { + spvtools::opt::Pass::Status LegalizeResourceArrayIndexPass::Process() { return m_mode == Mode::MarkLoopsForUnroll ? MarkLoopsForUnroll() : LowerToConstantSwitch(); } - spvtools::opt::Pass::Status LegalizeStorageBlockArrayIndexPass::MarkLoopsForUnroll() { + spvtools::opt::Pass::Status LegalizeResourceArrayIndexPass::MarkLoopsForUnroll() { auto* irContext = context(); const std::vector uses = CollectDynamicIndexUses(irContext); if (uses.empty()) { @@ -337,11 +371,11 @@ namespace MobileGL { if (!modified) { return Status::SuccessWithoutChange; } - MGLOG_D("[spirv] storage-block array index: marked enclosing loops for full unrolling"); + MGLOG_D("[spirv] resource array index: marked enclosing loops for full unrolling"); return Status::SuccessWithChange; } - spvtools::opt::Pass::Status LegalizeStorageBlockArrayIndexPass::LowerToConstantSwitch() { + spvtools::opt::Pass::Status LegalizeResourceArrayIndexPass::LowerToConstantSwitch() { auto* irContext = context(); bool modified = false; @@ -357,7 +391,8 @@ namespace MobileGL { if (declined.count(use.accessChain->result_id()) != 0) { continue; } - const LoweringOutcome outcome = LowerOneChain(use.accessChain, use.arrayLength); + const LoweringOutcome outcome = + LowerOneChain(use.accessChain, use.arrayLength, use.isImageArray); if (outcome == LoweringOutcome::Declined) { declined.insert(use.accessChain->result_id()); continue; @@ -383,17 +418,21 @@ namespace MobileGL { return Status::SuccessWithChange; } - LegalizeStorageBlockArrayIndexPass::LoweringOutcome - LegalizeStorageBlockArrayIndexPass::LowerOneChain(Instruction* accessChain, uint32_t arrayLength) { + LegalizeResourceArrayIndexPass::LoweringOutcome + LegalizeResourceArrayIndexPass::LowerOneChain(Instruction* accessChain, uint32_t arrayLength, + bool isImageArray) { auto* irContext = context(); if (arrayLength == 0 || arrayLength > kMaxLoweredArrayLength) { - MGLOG_D("[spirv] storage-block array index: array length %u is not lowerable", + MGLOG_D("[spirv] resource array index: array length %u is not lowerable", arrayLength); return LoweringOutcome::Declined; } if (!IsLowerableIndexType(irContext, accessChain->GetSingleWordInOperand(1))) { return LoweringOutcome::Declined; } + if (isImageArray) { + return LowerImageChain(accessChain, arrayLength); + } std::vector stores; std::vector loads; @@ -458,8 +497,8 @@ namespace MobileGL { // every path. An index outside [0, length) reaches the default target, which is // the merge block: nothing is stored, which is what indexing a block array out of // range already meant. - LegalizeStorageBlockArrayIndexPass::LoweringOutcome - LegalizeStorageBlockArrayIndexPass::LowerStore(Instruction* accessChain, uint32_t arrayLength, + LegalizeResourceArrayIndexPass::LoweringOutcome + LegalizeResourceArrayIndexPass::LowerStore(Instruction* accessChain, uint32_t arrayLength, Instruction* store) { auto* irContext = context(); BasicBlock* block = irContext->get_instr_block(store); @@ -543,8 +582,8 @@ namespace MobileGL { // pick with OpSelect. Reading the elements the shader did not ask for is safe - // every one of them is a storage block this stage already declares, and an ES // driver bounds-checks a storage buffer read that lands outside what is bound. - LegalizeStorageBlockArrayIndexPass::LoweringOutcome - LegalizeStorageBlockArrayIndexPass::LowerLoad(Instruction* accessChain, uint32_t arrayLength, + LegalizeResourceArrayIndexPass::LoweringOutcome + LegalizeResourceArrayIndexPass::LowerLoad(Instruction* accessChain, uint32_t arrayLength, Instruction* load) { auto* irContext = context(); uint32_t conditionTypeId = 0; @@ -594,16 +633,283 @@ namespace MobileGL { return LoweringOutcome::Changed; } - spvtools::Optimizer::PassToken - LegalizeStorageBlockArrayIndexPass::CreateMarkLoopsForUnrollPass() { - return spvtools::Optimizer::PassToken( - MakeUnique(Mode::MarkLoopsForUnroll)); + // An image array's chain is never stored or loaded THROUGH the way a storage + // block's is: it is OpLoad-ed once into an opaque image object, and the image ops + // consume that object. So this resolves the chain one CONSUMER at a time - the + // round loop in LowerToConstantSwitch recollects after each - and refuses anything + // that is not a plain read or write of the loaded image. + LegalizeResourceArrayIndexPass::LoweringOutcome + LegalizeResourceArrayIndexPass::LowerImageChain(Instruction* accessChain, uint32_t arrayLength) { + auto* irContext = context(); + + std::vector loads; + bool unsupportedUse = false; + irContext->get_def_use_mgr()->ForEachUser(accessChain, [&](Instruction* user) { + switch (user->opcode()) { + case spv::Op::OpName: + case spv::Op::OpDecorate: + case spv::Op::OpDecorateId: + return; + case spv::Op::OpLoad: + // Memory operands would be dropped by the per-element rebuild, so a + // load carrying any is refused instead. + if (user->NumInOperands() == 1) { + loads.push_back(user); + } else { + unsupportedUse = true; + } + return; + default: + // OpImageTexelPointer above all: that is how an imageAtomic* reaches + // the array, and running one per element would perform every OTHER + // element's atomic as well - a read can be thrown away, a + // read-modify-write cannot. + unsupportedUse = true; + return; + } + }); + + if (unsupportedUse) { + MGLOG_D("[spirv] image array index: chain %%%u has a use this pass cannot rewrite", + accessChain->result_id()); + return LoweringOutcome::Declined; + } + + if (loads.empty()) { + // No uses left: the chain itself is what detection is still seeing. + irContext->KillInst(accessChain); + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + return LoweringOutcome::Changed; + } + + Instruction* load = loads.front(); + Instruction* consumer = nullptr; + bool unsupportedConsumer = false; + irContext->get_def_use_mgr()->ForEachUser(load, [&](Instruction* user) { + switch (user->opcode()) { + case spv::Op::OpName: + case spv::Op::OpDecorate: + case spv::Op::OpDecorateId: + return; + case spv::Op::OpImageWrite: + case spv::Op::OpImageRead: + if (consumer == nullptr) consumer = user; + return; + default: + // A sampled-image construction, a query, a copy, an argument to a + // function: shapes whose per-element rebuild this pass cannot spell + // exactly. + unsupportedConsumer = true; + return; + } + }); + + if (unsupportedConsumer) { + MGLOG_D("[spirv] image array index: the image loaded from chain %%%u is consumed by " + "an operation this pass cannot rewrite", + accessChain->result_id()); + return LoweringOutcome::Declined; + } + if (consumer == nullptr) { + irContext->KillInst(load); + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + return LoweringOutcome::Changed; + } + + return consumer->opcode() == spv::Op::OpImageWrite + ? LowerImageWrite(accessChain, arrayLength, load, consumer) + : LowerImageRead(accessChain, arrayLength, load, consumer); + } + + // Drops |load| and |accessChain| once the rewrite above has taken their last user, + // in that order - the load is what uses the chain. Anything still using either is + // another consumer a later round will come back for. + void LegalizeResourceArrayIndexPass::KillImageChainIfDead(Instruction* accessChain, + Instruction* load) { + auto* irContext = context(); + if (irContext->get_def_use_mgr()->NumUsers(load) == 0) { + irContext->KillInst(load); + } + if (irContext->get_def_use_mgr()->NumUsers(accessChain) == 0) { + irContext->KillInst(accessChain); + } + } + + // switch (idx) { case 0: imageStore(arr[0], ...); break; case 1: ... } + // + // The same block split as LowerStore, for the same reason: whatever followed the + // write still runs exactly once on every path, and an index outside [0, length) + // reaches the default target - the merge block - so nothing is written, which is + // what indexing an image array out of range already meant. + LegalizeResourceArrayIndexPass::LoweringOutcome + LegalizeResourceArrayIndexPass::LowerImageWrite(Instruction* accessChain, uint32_t arrayLength, + Instruction* load, Instruction* imageWrite) { + auto* irContext = context(); + BasicBlock* block = irContext->get_instr_block(imageWrite); + if (block == nullptr) { + return LoweringOutcome::Declined; + } + // Splitting a loop header keeps the label - and so the back edge's target - on + // the first half while the OpLoopMerge moves to the second, which is not a loop + // any more. Refuse instead of producing that. + if (block->GetLoopMergeInst() != nullptr) { + MGLOG_D("[spirv] image array index: write sits in a loop header, declining"); + return LoweringOutcome::Declined; + } + Function* function = block->GetParent(); + if (function == nullptr) { + return LoweringOutcome::Declined; + } + + const uint32_t indexId = accessChain->GetSingleWordInOperand(1); + const uint32_t imageTypeId = load->type_id(); + // Coordinate, texel and any image operands, verbatim: only the image itself is + // per-element. + std::vector tailOperands; + for (uint32_t i = 1; i < imageWrite->NumInOperands(); ++i) { + tailOperands.push_back(imageWrite->GetInOperand(i)); + } + + const uint32_t mergeLabelId = irContext->TakeNextId(); + block->SplitBasicBlock(irContext, mergeLabelId, BasicBlock::iterator(imageWrite)); + // |imageWrite| now heads the merge block; the per-element writes replace it. + irContext->KillInst(imageWrite); + + std::vector> targets; + targets.reserve(arrayLength); + BasicBlock* insertAfter = block; + for (uint32_t element = 0; element < arrayLength; ++element) { + const uint32_t caseLabelId = irContext->TakeNextId(); + auto caseBlock = MakeUnique(MakeUnique( + irContext, spv::Op::OpLabel, 0, caseLabelId, std::initializer_list{})); + caseBlock->SetParent(function); + BasicBlock* casePtr = function->InsertBasicBlockAfter(std::move(caseBlock), insertAfter); + // Hand-built label; see LowerStore for why it has to be registered here. + irContext->AnalyzeDefUse(casePtr->GetLabelInst()); + irContext->set_instr_block(casePtr->GetLabelInst(), casePtr); + + InstructionBuilder caseBuilder( + irContext, casePtr, + IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + const uint32_t constantId = ConstantLikeIndex(irContext, indexId, element); + Instruction* elementChain = + CloneChainWithConstantIndex(caseBuilder, irContext, accessChain, constantId); + Instruction* elementImage = + caseBuilder.AddLoad(imageTypeId, elementChain->result_id()); + + std::vector writeOperands; + writeOperands.push_back({SPV_OPERAND_TYPE_ID, {elementImage->result_id()}}); + for (const Operand& tailOperand : tailOperands) { + writeOperands.push_back(tailOperand); + } + caseBuilder.AddInstruction( + MakeUnique(irContext, spv::Op::OpImageWrite, 0, 0, writeOperands)); + caseBuilder.AddBranch(mergeLabelId); + + targets.push_back({Operand::OperandData{element}, caseLabelId}); + insertAfter = casePtr; + } + + InstructionBuilder switchBuilder( + irContext, block, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + switchBuilder.AddSwitch(indexId, mergeLabelId, targets, mergeLabelId); + + KillImageChainIfDead(accessChain, load); + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + MGLOG_D("[spirv] image array index: lowered a dynamic imageStore to a %u-way switch", + arrayLength); + return LoweringOutcome::Changed; + } + + // A read needs no control flow: read every element through a constant index and pick + // with OpSelect. The selection happens on the RESULT, not on the image object - an + // opaque type may not be selected at all (pre-1.4 OpSelect takes pointers, scalars + // and vectors only, and ESSL has no ternary on an image), so what is duplicated is + // the OpImageRead. + // + // Reading the elements the shader did not ask for is safe: every one of them is an + // image this stage already declares, and GL 4.6 7.11.2 makes a load through an + // image unit whose binding is missing or incompatible return undefined DATA - never + // an error, and never a fault - which the select then discards. Contrast an + // imageAtomic*, which LowerImageChain refuses for exactly the opposite reason. + LegalizeResourceArrayIndexPass::LoweringOutcome + LegalizeResourceArrayIndexPass::LowerImageRead(Instruction* accessChain, uint32_t arrayLength, + Instruction* load, Instruction* imageRead) { + auto* irContext = context(); + uint32_t conditionTypeId = 0; + uint32_t dimension = 0; + if (!TryGetSelectConditionType(irContext, imageRead->type_id(), &conditionTypeId, + &dimension)) { + MGLOG_D("[spirv] image array index: read type is not selectable, declining"); + return LoweringOutcome::Declined; + } + const uint32_t boolTypeId = irContext->get_type_mgr()->GetBoolTypeId(); + const uint32_t indexId = accessChain->GetSingleWordInOperand(1); + const uint32_t imageTypeId = load->type_id(); + std::vector tailOperands; + for (uint32_t i = 1; i < imageRead->NumInOperands(); ++i) { + tailOperands.push_back(imageRead->GetInOperand(i)); + } + + InstructionBuilder builder( + irContext, imageRead, + IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); + + uint32_t selectedId = 0; + for (uint32_t element = 0; element < arrayLength; ++element) { + const uint32_t constantId = ConstantLikeIndex(irContext, indexId, element); + Instruction* elementChain = + CloneChainWithConstantIndex(builder, irContext, accessChain, constantId); + Instruction* elementImage = builder.AddLoad(imageTypeId, elementChain->result_id()); + + std::vector readOperands; + readOperands.push_back({SPV_OPERAND_TYPE_ID, {elementImage->result_id()}}); + for (const Operand& tailOperand : tailOperands) { + readOperands.push_back(tailOperand); + } + Instruction* elementRead = builder.AddInstruction( + MakeUnique(irContext, spv::Op::OpImageRead, imageRead->type_id(), + irContext->TakeNextId(), readOperands)); + if (element == 0) { + // Element 0 is the else-arm of the whole ladder, so an out-of-range + // index reads it - an undefined element for an undefined index. + selectedId = elementRead->result_id(); + continue; + } + + Instruction* isElement = + builder.AddBinaryOp(boolTypeId, spv::Op::OpIEqual, indexId, constantId); + uint32_t conditionId = isElement->result_id(); + if (dimension > 1) { + std::vector components(dimension, conditionId); + conditionId = builder.AddCompositeConstruct(conditionTypeId, components)->result_id(); + } + selectedId = builder + .AddSelect(imageRead->type_id(), conditionId, + elementRead->result_id(), selectedId) + ->result_id(); + } + + irContext->ReplaceAllUsesWith(imageRead->result_id(), selectedId); + irContext->KillInst(imageRead); + KillImageChainIfDead(accessChain, load); + irContext->InvalidateAnalysesExceptFor(IRContext::kAnalysisNone); + MGLOG_D("[spirv] image array index: lowered a dynamic imageLoad to %u constant-indexed " + "reads", + arrayLength); + return LoweringOutcome::Changed; } spvtools::Optimizer::PassToken - LegalizeStorageBlockArrayIndexPass::CreateLowerToConstantSwitchPass() { + LegalizeResourceArrayIndexPass::CreateMarkLoopsForUnrollPass() { return spvtools::Optimizer::PassToken( - MakeUnique(Mode::LowerToConstantSwitch)); + MakeUnique(Mode::MarkLoopsForUnroll)); + } + + spvtools::Optimizer::PassToken + LegalizeResourceArrayIndexPass::CreateLowerToConstantSwitchPass() { + return spvtools::Optimizer::PassToken( + MakeUnique(Mode::LowerToConstantSwitch)); } } // namespace ShaderTranspiler } // namespace MG_Util diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.h new file mode 100644 index 00000000..f77d542d --- /dev/null +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.h @@ -0,0 +1,157 @@ +// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeResourceArrayIndexPass.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#pragma once +#include "source/opt/pass.h" +#include "spirv-tools/optimizer.hpp" + +#include +#include + +namespace MobileGL { + namespace MG_Util { + namespace ShaderTranspiler { + // Desktop GL lets an ARRAY OF SHADER STORAGE BLOCKS and an ARRAY OF IMAGE UNIFORMS + // alike be indexed with any dynamically-uniform expression (GL 4.6 core / GLSL 4.30 + // 4.1.9). GLSL ES keeps the stricter ES 3.1 rule for BOTH - the index must be a + // *constant integral expression* - and the drivers enforce it to the letter: + // + // Qualcomm, storage blocks: + // '[' : indexing into an SSBO array using a non-constant expression is not + // permitted + // Mesa, images: + // image arrays indexed with non-constant expressions are forbidden in GLSL ES + // + // glslang keeps the whole array as ONE SPIR-V variable, so SPIRV-Cross prints + // `layout(binding = N, std430) buffer Blk { ... } arr[4];` plus `arr[i]` - or + // `uniform image2D g_image[4];` plus `imageStore(g_image[i], ...)` - verbatim, and + // the stage never compiles. The backend program then links nothing and every draw + // or dispatch that uses it is a silent no-op, which reads back as "the buffer was + // never written" rather than as an error - the frontend has already published + // GL_LINK_STATUS = TRUE from glslang's own link. + // + // Verified on the device for the storage-block half: an Adreno 830 ES probe with no + // MobileGL in the loop rejects the non-constant subscript with AND without + // GL_EXT_gpu_shader5 (which the driver does advertise), and accepts a constant one. + // Verified again for the image half on llvmpipe / Mesa 26.1.4 at ES 3.2, on a raw + // GLES probe: a scalar image and an array with literal subscripts both write the + // units they name, and both a loop-variable subscript and a `const int[]` table + // lookup are refused with the message above. So the ES 3.2 "dynamically uniform" + // relaxation is not a way out for either resource - every index really has to + // become a compile-time constant. + // + // SAMPLER arrays are deliberately NOT covered. ESSL 3.20 4.1.7 does allow a sampler + // array a dynamically-uniform index, and the same probe confirms it: a sampler array + // subscripted by a loop variable, and one reached through a const table, both compile + // and link. Lowering them would cost code for a rule that does not exist. + // + // Two modes, used as two halves of one legalization in + // ShaderCompiler::LegalizeResourceArrayIndexingForEssl - the same shape, for + // the same reasons, as LegalizeFragmentOutputIndexPass: + // + // MarkLoopsForUnroll - `for (int i = 0; i < 4; ++i) arr[i].x = ...` is the + // common shape, and full unrolling turns its index into a literal at no cost + // in emitted code. spirv-opt's CreateLoopUnrollPass only touches loops whose + // OpLoopMerge carries the Unroll control, so this mode sets that hint on + // exactly the loops that enclose an offending access chain, and only when + // their trip count is known and small. Must run AFTER ssa-rewrite: both the + // trip-count check and the unroller need the induction variable as an OpPhi. + // Resource-kind-blind: the offending chain is the same instruction either way. + // + // LowerToConstantSwitch - the fallback for a genuinely dynamic index + // (uniform-sourced, which is what the CTS indirect-addressing and resource-max + // cases use). A write through such a chain becomes an OpSwitch over the + // array's range with one constant-indexed access per case; a read becomes one + // constant-indexed access per element combined with OpSelect. This is what ANGLE + // does for the same ES 3.1 rule. + // + // This half IS kind-specific, because the two resources are consumed + // differently. A storage block is reached by OpStore/OpLoad THROUGH the access + // chain, so the chain's own users are rewritten. An image's access chain is + // first OpLoad-ed into an opaque image OBJECT, which OpImageWrite/OpImageRead + // then consume - and an opaque type may not be selected (OpSelect is restricted + // to pointers, scalars and vectors before SPIR-V 1.4, and ESSL has no ternary on + // image types at all), so it is the image OPERATION that is duplicated per + // element, not the loaded object. + // + // A UNIFORM block array is a different namespace with its own (less strictly + // enforced) rule and no observed failure, so it is deliberately left alone rather + // than lowered on speculation. + // + // DirectGLES transpile path only: the original module is legal for Vulkan, which + // has no such restriction, and DirectVulkan must keep seeing the array as one + // descriptor array. + // + // The pass DECLINES - leaving the module untouched rather than half-transforming + // it - whenever it meets a shape it cannot rewrite exactly: a pointer handed to a + // function or chained further, an atomic or an OpArrayLength through the chain, a + // load carrying memory operands, a spec-constant array length, an index that is + // not a 32-bit integer, an image operation other than a plain read or write (an + // OpImageTexelPointer, i.e. an imageAtomic*, above all - executing it per element + // would perform the other elements' atomics too), or a store sitting in a loop + // header block (splitting there would move the OpLoopMerge away from the back + // edge's target). + class LegalizeResourceArrayIndexPass final : public spvtools::opt::Pass { + public: + enum class Mode { + MarkLoopsForUnroll, + LowerToConstantSwitch, + }; + + explicit LegalizeResourceArrayIndexPass(Mode mode) : m_mode(mode) {} + + const char* name() const override { + return m_mode == Mode::MarkLoopsForUnroll + ? "mobilegl-mark-resource-array-index-loops" + : "mobilegl-lower-resource-array-index"; + } + + Status Process() override; + + static spvtools::Optimizer::PassToken CreateMarkLoopsForUnrollPass(); + static spvtools::Optimizer::PassToken CreateLowerToConstantSwitchPass(); + + // The detection half, on a serialized module: true when an array of storage + // blocks or of image uniforms is indexed with anything but an OpConstant. Cheap + // enough to gate the whole legalization on (one BuildModule, no serialization) + // and used again after the folding chain to decide whether the fallback has to + // run at all. + static bool BinaryHasDynamicResourceArrayIndexing(const std::vector& binary); + + private: + enum class LoweringOutcome { + // The shape is not one this pass can rewrite exactly; the module keeps + // the illegal chain rather than a half-transform of it. + Declined, + Changed, + }; + + Status MarkLoopsForUnroll(); + Status LowerToConstantSwitch(); + + LoweringOutcome LowerOneChain(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + bool isImageArray); + LoweringOutcome LowerStore(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + spvtools::opt::Instruction* store); + LoweringOutcome LowerLoad(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + spvtools::opt::Instruction* load); + LoweringOutcome LowerImageChain(spvtools::opt::Instruction* accessChain, uint32_t arrayLength); + void KillImageChainIfDead(spvtools::opt::Instruction* accessChain, + spvtools::opt::Instruction* load); + LoweringOutcome LowerImageWrite(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + spvtools::opt::Instruction* load, + spvtools::opt::Instruction* imageWrite); + LoweringOutcome LowerImageRead(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, + spvtools::opt::Instruction* load, + spvtools::opt::Instruction* imageRead); + + Mode m_mode; + }; + } // namespace ShaderTranspiler + } // namespace MG_Util +} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h deleted file mode 100644 index 390acc15..00000000 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h +++ /dev/null @@ -1,120 +0,0 @@ -// MobileGL - MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/LegalizeStorageBlockArrayIndexPass.h -// Copyright (c) 2025-2026 MobileGL-Dev -// Licensed under the GNU Lesser General Public License v3.0: -// https://www.gnu.org/licenses/gpl-3.0.txt -// https://www.gnu.org/licenses/lgpl-3.0.txt -// SPDX-License-Identifier: LGPL-3.0-only -// End of Source File Header - -#pragma once -#include "source/opt/pass.h" -#include "spirv-tools/optimizer.hpp" - -#include -#include - -namespace MobileGL { - namespace MG_Util { - namespace ShaderTranspiler { - // GL 4.3 lets an ARRAY OF SHADER STORAGE BLOCKS be indexed with any - // dynamically-uniform expression (GL 4.6 core / GLSL 4.30 4.1.9). GLSL ES keeps - // the stricter ES 3.1 rule - the index must be a *constant integral expression* - - // and the Qualcomm ES compiler enforces it to the letter: - // - // '[' : indexing into an SSBO array using a non-constant expression is not - // permitted - // - // glslang keeps the whole array as ONE SPIR-V variable, so SPIRV-Cross prints - // `layout(binding = N, std430) buffer Blk { ... } arr[4];` plus `arr[i]` verbatim - // and the stage never compiles. The backend program then links nothing and every - // draw or dispatch that uses it is a silent no-op, which reads back as "the buffer - // was never written" rather than as an error - the frontend has already published - // GL_LINK_STATUS = TRUE from glslang's own link. - // - // Verified on the device: an Adreno 830 ES probe with no MobileGL in the loop - // rejects the non-constant subscript with AND without GL_EXT_gpu_shader5 (which - // the driver does advertise), and accepts a constant one. So the ES 3.2 - // "dynamically uniform" relaxation is not a way out - every index really has to - // become a compile-time constant. - // - // Two modes, used as two halves of one legalization in - // ShaderCompiler::LegalizeStorageBlockArrayIndexingForEssl - the same shape, for - // the same reasons, as LegalizeFragmentOutputIndexPass: - // - // MarkLoopsForUnroll - `for (int i = 0; i < 4; ++i) arr[i].x = ...` is the - // common shape, and full unrolling turns its index into a literal at no cost - // in emitted code. spirv-opt's CreateLoopUnrollPass only touches loops whose - // OpLoopMerge carries the Unroll control, so this mode sets that hint on - // exactly the loops that enclose an offending access chain, and only when - // their trip count is known and small. Must run AFTER ssa-rewrite: both the - // trip-count check and the unroller need the induction variable as an OpPhi. - // - // LowerToConstantSwitch - the fallback for a genuinely dynamic index - // (uniform-sourced, which is what the CTS indirect-addressing and resource-max - // cases use). A write through such a chain becomes an OpSwitch over the - // array's range with one constant-indexed store per case; a read becomes one - // constant-indexed load per element combined with OpSelect. This is what ANGLE - // does for the same ES 3.1 rule. - // - // Storage blocks only. A UNIFORM block array is a different namespace with its own - // (less strictly enforced) rule and no observed failure, so it is deliberately left - // alone rather than lowered on speculation. - // - // DirectGLES transpile path only: the original module is legal for Vulkan, which - // has no such restriction, and DirectVulkan must keep seeing the array as one - // descriptor array. - // - // The pass DECLINES - leaving the module untouched rather than half-transforming - // it - whenever it meets a shape it cannot rewrite exactly: a pointer handed to a - // function or chained further, an atomic or an OpArrayLength through the chain, a - // load carrying memory operands, a spec-constant array length, an index that is - // not a 32-bit integer, or a store sitting in a loop header block (splitting there - // would move the OpLoopMerge away from the back edge's target). - class LegalizeStorageBlockArrayIndexPass final : public spvtools::opt::Pass { - public: - enum class Mode { - MarkLoopsForUnroll, - LowerToConstantSwitch, - }; - - explicit LegalizeStorageBlockArrayIndexPass(Mode mode) : m_mode(mode) {} - - const char* name() const override { - return m_mode == Mode::MarkLoopsForUnroll - ? "mobilegl-mark-storage-block-array-index-loops" - : "mobilegl-lower-storage-block-array-index"; - } - - Status Process() override; - - static spvtools::Optimizer::PassToken CreateMarkLoopsForUnrollPass(); - static spvtools::Optimizer::PassToken CreateLowerToConstantSwitchPass(); - - // The detection half, on a serialized module: true when an array of storage - // blocks is indexed with anything but an OpConstant. Cheap enough to gate the - // whole legalization on (one BuildModule, no serialization) and used again - // after the folding chain to decide whether the fallback has to run at all. - static bool BinaryHasDynamicStorageBlockArrayIndexing(const std::vector& binary); - - private: - enum class LoweringOutcome { - // The shape is not one this pass can rewrite exactly; the module keeps - // the illegal chain rather than a half-transform of it. - Declined, - Changed, - }; - - Status MarkLoopsForUnroll(); - Status LowerToConstantSwitch(); - - LoweringOutcome LowerOneChain(spvtools::opt::Instruction* accessChain, uint32_t arrayLength); - LoweringOutcome LowerStore(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, - spvtools::opt::Instruction* store); - LoweringOutcome LowerLoad(spvtools::opt::Instruction* accessChain, uint32_t arrayLength, - spvtools::opt::Instruction* load); - - Mode m_mode; - }; - } // namespace ShaderTranspiler - } // namespace MG_Util -} // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h b/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h index 45f89a22..43f989fa 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h +++ b/MobileGL/MG_Util/ShaderTranspiler/TranslationCache.h @@ -566,7 +566,7 @@ namespace MobileGL::MG_Util::ShaderTranspiler { // // Unconditional passes take no input but the module and so need no key material: // StripUboMemberRelaxedPrecision, LowerRectImages, Lower1DArrayImages, - // Lower1DSampledImages, LegalizeStorageBlockArrayIndexing and + // Lower1DSampledImages, LegalizeResourceArrayIndexing and // FlattenAtomicCounterBlockOffsets. Each self-gates on the module's own content and is // armed by nothing, so the SPIR-V already in this key covers them completely. //