diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index c7c2bfd8..3b157bfe 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -6437,11 +6437,12 @@ namespace MobileGL::MG_Backend::DirectGLES { // declaration and preserves its binding - an image unit cannot be set from // the API in ES, so the qualifier is the only binding mechanism there is, // and both halves of the pair have to still be carrying theirs when it runs. - // * and it needs the STAGE, because the qualifier it adds is a per-stage decision - // and the rename that keeps two stages from declaring one image uniform - // differently is keyed on it. + // Takes no stage: the qualifier it adds is a decision about THIS text's accesses + // and the rename that keeps two stages from declaring one image uniform + // differently is keyed on that same decision, so two stages that agree still + // share one uniform (see the location-budget note on the pass). Uint splitImageUniformCount = 0; - source = SplitReadWriteImageUniforms(source, glShaderType, &splitImageUniformCount); + source = SplitReadWriteImageUniforms(source, &splitImageUniformCount); if (splitImageUniformCount != 0) { splitImageUniformStages.push_back({shader->GetShaderStage(), splitImageUniformCount}); } diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index b9af004c..7237a556 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -933,7 +933,7 @@ namespace MobileGL::MG_Backend::DirectGLES { struct ImageUniformDecl { String name; - String stageName; // the stage-tagged name the rewritten declaration takes; empty + String aliasName; // the repair-tagged name the rewritten declaration takes; empty // for a declaration this pass leaves alone String writeName; // the writeonly half's name, when split String layout; // raw contents of layout(...) @@ -1029,21 +1029,6 @@ namespace MobileGL::MG_Backend::DirectGLES { } } // namespace - String ImageStageAliasPrefix(GLenum shaderType) { - switch (shaderType) { - case GL_VERTEX_SHADER: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Vs_"; - case GL_FRAGMENT_SHADER: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Fs_"; - case GL_COMPUTE_SHADER: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Cs_"; - case GL_GEOMETRY_SHADER: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Gs_"; - case GL_TESS_CONTROL_SHADER: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Tcs_"; - case GL_TESS_EVALUATION_SHADER: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Tes_"; - // A stage this build does not know. Still a tag of its own rather than the bare - // name, so the anti-collision property below never depends on the switch being - // exhaustive - it only stops being able to say WHICH stage a name came from. - default: return String(IMAGE_STAGE_ALIAS_PREFIX) + "Xs_"; - } - } - namespace { // The digits of an array extent or of an element subscript, or -1 for "not a plain // decimal literal". @@ -1260,7 +1245,7 @@ namespace MobileGL::MG_Backend::DirectGLES { return result; } - String SplitReadWriteImageUniforms(const String& glslCode, GLenum shaderType, Uint* outSplitCount) { + String SplitReadWriteImageUniforms(const String& glslCode, Uint* outSplitCount) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif @@ -1405,38 +1390,52 @@ namespace MobileGL::MG_Backend::DirectGLES { Vector edits; Vector takenNames; - const String stagePrefix = ImageStageAliasPrefix(shaderType); for (auto& decl : decls) { if (decl.unknownUse) continue; // leave it exactly as it was; no guessing - // EVERY declaration this pass rewrites is also RENAMED, per stage - the - // qualifier it is about to add is a per-stage decision, and GLSL requires a - // uniform declared in two stages to be declared IDENTICALLY (GLSL 4.3 4.3.9 / - // GLSL ES 3.20 4.3.9). A shader that stores to an image in the vertex stage and - // loads it in the fragment stage gets `writeonly` on one and `readonly` on the - // other, and on Adreno the linker merges the two same-named declarations and - // SILENTLY DISCARDS the vertex-stage stores: no GL error, no link log, - // LINK_STATUS = 1, and the image still holding its initial contents afterwards + // EVERY declaration this pass rewrites is also RENAMED, under the prefix of the + // repair it is about to receive - the qualifier below is a decision about ONE + // STAGE's accesses, and GLSL requires a uniform declared in two stages to be + // declared IDENTICALLY (GLSL 4.3 4.3.9 / GLSL ES 3.20 4.3.9). A shader that + // stores to an image in the vertex stage and loads it in the fragment stage gets + // `writeonly` on one and `readonly` on the other, and on Adreno the linker merges + // the two same-named declarations and SILENTLY DISCARDS the vertex-stage stores: + // no GL error, no link log, LINK_STATUS = 1, and the image still holding its + // initial contents afterwards // (KHR-GL4x.shader_image_load_store.advanced-memory-dependentInvocation, and any - // shader pack that writes an image in one stage to read it in another). A - // per-stage name means there is no cross-stage variable left to merge. + // shader pack that writes an image in one stage to read it in another). // - // Unconditional, rather than only when another stage is known to declare the same - // name, because this pass sees ONE stage at a time and a conditional rename would - // have to guess. Nothing downstream reads these names: the two passes that key on - // the GL uniform name (RebindImageUniformsToFrontendUnits, BakeImageFormatQualifiers) - // both run BEFORE this one, RemoveLayoutBinding recognises an image declaration by - // its TYPE token, and CacheResourceLocations skips image uniforms outright because - // ES image units come only from layout(binding=N). The declarations this pass - // LEAVES ALONE - already readonly/writeonly in the source, or r32f/r32i/r32ui, - // which need no qualifier - keep their names, and they are exactly the ones that - // already match across stages. - decl.stageName = MakeImageAliasName(stagePrefix, decl.name, glslCode, takenNames); - takenNames.push_back(decl.stageName); + // Keyed on the REPAIR and not on the stage, which is what makes the rename + // exactly as wide as the problem. Two stages that use the image the same way + // reach the same prefix and emit byte-identical declarations, so they keep ONE + // shared uniform and there is nothing mismatched to merge; two that use it + // differently reach different prefixes and cannot be merged at all. Tagging by + // stage instead also broke the merge - but it broke it for the agreeing stages + // too, turning one image uniform into one PER STAGE that names it, and Adreno + // allocates image locations per distinct uniform: the five stages of + // KHR-GL43.shading_language_420pack.binding_images_texture_type_* went from 6 + // image uniforms to 30 and the link failed outright with "Error: Image Image + // location or component exceeds max allowed." on an Adreno 830, where Mali and + // Mesa both accept the same text. + // + // Nothing downstream reads these names: the two passes that key on the GL uniform + // name (RebindImageUniformsToFrontendUnits, BakeImageFormatQualifiers) both run + // BEFORE this one, RemoveLayoutBinding recognises an image declaration by its TYPE + // token, and CacheResourceLocations skips image uniforms outright because ES image + // units come only from layout(binding=N). The declarations this pass LEAVES ALONE - + // already readonly/writeonly in the source, or r32f/r32i/r32ui, which need no + // qualifier - keep their names, and they are exactly the ones that already match + // across stages. + const char* aliasPrefix = decl.loaded && decl.stored ? IMAGE_SPLIT_READ_ALIAS_PREFIX + : decl.stored ? IMAGE_WRITEONLY_ALIAS_PREFIX + : IMAGE_READONLY_ALIAS_PREFIX; + decl.aliasName = MakeImageAliasName(aliasPrefix, decl.name, glslCode, takenNames); + takenNames.push_back(decl.aliasName); if (decl.loaded && decl.stored) { - // Minted from the ALREADY stage-tagged name, so two stages that both split - // the same image do not collide on the write half either. + // Minted from the ALREADY access-tagged name, so the write half of a split + // can never collide with the single declaration another stage's repair mints + // for the same image. decl.writeName = - MakeImageAliasName(IMAGE_WRITE_ALIAS_PREFIX, decl.stageName, glslCode, takenNames); + MakeImageAliasName(IMAGE_WRITE_ALIAS_PREFIX, decl.aliasName, glslCode, takenNames); takenNames.push_back(decl.writeName); decl.split = true; if (outSplitCount != nullptr) ++*outSplitCount; @@ -1445,28 +1444,28 @@ namespace MobileGL::MG_Backend::DirectGLES { // there is no visibility to restore and no reason to pay for the cache // behaviour. edits.push_back({decl.declStart, decl.declLength, - BuildImageDeclaration(decl, "readonly", decl.stageName, + BuildImageDeclaration(decl, "readonly", decl.aliasName, /*forceCoherent=*/true) + "\n" + BuildImageDeclaration(decl, "writeonly", decl.writeName, /*forceCoherent=*/true)}); } else if (decl.stored) { edits.push_back({decl.declStart, decl.declLength, - BuildImageDeclaration(decl, "writeonly", decl.stageName)}); + BuildImageDeclaration(decl, "writeonly", decl.aliasName)}); } else { // Loaded only, or only ever handed to imageSize (or unused): readonly is // the qualifier that keeps every one of those legal. edits.push_back({decl.declStart, decl.declLength, - BuildImageDeclaration(decl, "readonly", decl.stageName)}); + BuildImageDeclaration(decl, "readonly", decl.aliasName)}); } } for (const ImageUseSite& site : useSites) { const ImageUniformDecl& decl = decls[site.declIndex]; // Empty exactly when the declaration was poisoned above and left untouched; its // uses must keep naming the variable that is still called that. - if (decl.stageName.empty()) continue; + if (decl.aliasName.empty()) continue; edits.push_back( - {site.start, site.length, decl.split && site.stores ? decl.writeName : decl.stageName}); + {site.start, site.length, decl.split && site.stores ? decl.writeName : decl.aliasName}); if (!decl.split || !site.stores) continue; // ...and an explicit barrier behind it. `coherent` on both halves is what makes // the store VISIBLE to a load through the other variable, but it says nothing diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index ea9f8543..8d9d0dcc 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -332,15 +332,16 @@ namespace MobileGL::MG_Backend::DirectGLES { const String& inPerVertexMembers, const String& outPerVertexMembers); // Prefix of the writeonly half a read+write image uniform is split into (see - // SplitReadWriteImageUniforms); the suffix is the image's own (already stage-tagged) name. + // SplitReadWriteImageUniforms); the suffix is the image's own (already access-tagged) name. constexpr const char* IMAGE_WRITE_ALIAS_PREFIX = "mg_imageWrite_"; - // Stem of the per-stage name every image declaration SplitReadWriteImageUniforms rewrites - // is renamed under; ImageStageAliasPrefix appends the stage tag and the separator. - constexpr const char* IMAGE_STAGE_ALIAS_PREFIX = "mg_image"; - // "mg_imageVs_", "mg_imageFs_", "mg_imageCs_", ... - the prefix SplitReadWriteImageUniforms - // renames a rewritten image declaration under, so that no two stages can end up declaring - // the same image uniform name with different memory qualifiers. Exposed for the tests. - String ImageStageAliasPrefix(GLenum shaderType); + // The three names SplitReadWriteImageUniforms renames a rewritten image declaration + // under, one per REPAIR it can apply. Which one a stage picks is decided by that stage's + // own accesses, so two stages that use an image the same way arrive at the SAME name and + // two that use it differently arrive at different ones - which is exactly the property + // the rename exists for, at no cost to the stages that agree. Exposed for the tests. + constexpr const char* IMAGE_READONLY_ALIAS_PREFIX = "mg_imageRo_"; + constexpr const char* IMAGE_WRITEONLY_ALIAS_PREFIX = "mg_imageWo_"; + constexpr const char* IMAGE_SPLIT_READ_ALIAS_PREFIX = "mg_imageRw_"; // ESSL refuses an image variable that carries a format qualifier other than r32f / // r32i / r32ui unless it also carries `readonly` or `writeonly` (GLSL ES 3.10 4.9 / // 3.20 4.10; glslang enforces it verbatim in ParseHelper.cpp's layoutObjectCheck). @@ -352,31 +353,47 @@ namespace MobileGL::MG_Backend::DirectGLES { // bare declaration, so the frontend raises no error and the illegal ESSL only shows // up as a device compile failure - and then as a silently no-op draw. // - // Restores a legal declaration, and RENAMES it per stage while doing so: - // * loaded only -> add `readonly` - // * stored only -> add `writeonly` + // Restores a legal declaration, and RENAMES it after the repair it applied while doing so: + // * loaded only -> add `readonly`, rename under IMAGE_READONLY_ALIAS_PREFIX + // * stored only -> add `writeonly`, rename under IMAGE_WRITEONLY_ALIAS_PREFIX // * both -> emit TWO declarations on the same binding and of the - // same type, `coherent readonly ` and `coherent - // writeonly `, point + // same type, `coherent readonly + // ` and `coherent + // writeonly `, point // every imageStore at the second one, and follow each of // those stores with `memoryBarrierImage();`. Several image // variables may share an image unit as long as they have // the same type and format, which is exactly what the pair // is. // - // The rename (ImageStageAliasPrefix) is the other half of the repair and applies to all - // three cases. The qualifier chosen above is a decision about ONE STAGE's accesses, and - // GLSL requires a uniform declared in two stages to be declared identically - so a shader - // that stores an image from the vertex stage and loads it from the fragment stage came out - // of here `writeonly` in one and `readonly` in the other. Adreno merges the two same-named - // declarations and silently drops the vertex-stage STORES: no GL error, no link log, - // LINK_STATUS = 1, and the image still reads back its initial contents + // The rename is the other half of the repair and applies to all three cases. The qualifier + // chosen above is a decision about ONE STAGE's accesses, and GLSL requires a uniform + // declared in two stages to be declared identically - so a shader that stores an image from + // the vertex stage and loads it from the fragment stage came out of here `writeonly` in one + // and `readonly` in the other. Adreno merges the two same-named declarations and silently + // drops the vertex-stage STORES: no GL error, no link log, LINK_STATUS = 1, and the image + // still reads back its initial contents // (KHR-GL4x.shader_image_load_store.advanced-memory-dependentInvocation; a raw-ES probe // isolated the trigger to the same-name/mismatched-qualifier pair, and only when both - // carry `coherent`). A per-stage name leaves no cross-stage variable to merge. Every - // rewritten declaration is renamed, including the readonly half of a split pair; the - // declarations this pass leaves untouched keep their names, and those are exactly the ones - // that already agree across stages. + // carry `coherent`). Renaming leaves no cross-stage variable to merge. + // + // The name is keyed on the REPAIR, not on the stage, and that distinction is the whole + // point: two stages that use an image the same way emit byte-identical declarations, so + // letting them keep one shared name costs nothing and merging them is correct, while two + // stages that use it differently land on different prefixes and cannot be merged at all. + // A per-STAGE tag also satisfied the first requirement but violated the second: it made + // the SAME image a distinct uniform in every stage that named it, and Adreno allocates + // image LOCATIONS per distinct uniform. KHR-GL43.shading_language_420pack. + // binding_images_texture_type_* declares three read+write images in each of its five + // stages; merged that is 6 image uniforms, per-stage-tagged it is 30, and the Adreno 830 + // linker answered "Error: Image Image location or component exceeds max allowed. Error: + // Linking failed." - which, the frontend having already published LINK_STATUS = TRUE from + // glslang's link, surfaced only as every draw silently doing nothing and the images + // reading back zero. Mali and Mesa link the same text, so nothing but a device gate + // catches this. + // + // The declarations this pass leaves untouched keep their names, and those are exactly the + // ones that already agree across stages. // // The `coherent` on both halves of the pair is load-bearing, not decoration: GLSL only // guarantees a write through one image variable is visible to a read through a DIFFERENT @@ -401,16 +418,14 @@ namespace MobileGL::MG_Backend::DirectGLES { // // Runs on the transpiled ESSL, so it must see the bindings the frontend units were // already rewritten to and must run before those bindings are stripped - see the call - // site in Managers.cpp. It is downstream of the L2 shader-translation memo (which stores - // what SPIRV-Cross emitted, before any of these text passes), so `shaderType` steering the - // names it mints needs no entry in BuildEsslTranslationKey. + // site in Managers.cpp. Its output is a function of the emitted text alone - it needs no + // stage and no per-program state - so it adds nothing to BuildEsslTranslationKey either. // // `outSplitCount`, when given, receives the number of declarations that were actually // doubled - i.e. exactly how many image uniforms this stage gained over what the // application declared. Zero for every shader but a handful, and the only number the // budget note above can be reported with. - String SplitReadWriteImageUniforms(const String& glslCode, GLenum shaderType, - Uint* outSplitCount = nullptr); + String SplitReadWriteImageUniforms(const String& glslCode, Uint* outSplitCount = nullptr); // Prefix of the per-sampler float uniform that carries GL_TEXTURE_LOD_BIAS into // the shader (see EmulateTextureLodBias); the suffix is the sampler's own name. constexpr const char* LOD_BIAS_UNIFORM_PREFIX = "mg_lodBias_"; diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index d01298d0..09d03378 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -20,11 +20,12 @@ using MobileGL::MG_Backend::DirectGLES::PrgramImpl::BakeImageFormatQualifiers; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::BuildPassthroughTessControlEssl; 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_ARRAY_ELEMENT_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_READONLY_ALIAS_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_SPLIT_READ_ALIAS_PREFIX; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITE_ALIAS_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITEONLY_ALIAS_PREFIX; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ImageArrayUnitPlan; -using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ImageStageAliasPrefix; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemapImageArrayElementUnits; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemoveLayoutBinding; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestExtendedImageFormats; @@ -44,14 +45,14 @@ namespace { return count; } - // Every fixture below is a fragment shader unless it says otherwise. The pass tags the name - // of every declaration it rewrites with the stage it ran on, so the expectations have to - // spell the same tag. - constexpr GLenum kStage = GL_FRAGMENT_SHADER; - String StageAlias(const String& name) { return ImageStageAliasPrefix(kStage) + name; } - // The writeonly half is minted from the ALREADY stage-tagged name, so it carries both. + // The pass tags the name of every declaration it rewrites with the REPAIR it applied, so the + // expectations have to spell the tag that matches how the fixture uses the image. + String RoAlias(const String& name) { return String(IMAGE_READONLY_ALIAS_PREFIX) + name; } + String WoAlias(const String& name) { return String(IMAGE_WRITEONLY_ALIAS_PREFIX) + name; } + String RwAlias(const String& name) { return String(IMAGE_SPLIT_READ_ALIAS_PREFIX) + name; } + // The writeonly half is minted from the ALREADY access-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 SplitWriteAlias(const String& name) { return WriteAlias(RwAlias(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); @@ -74,13 +75,13 @@ void main() mg_FragColor = loaded; } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); // Both halves: same binding, same format, same type - which is what makes two image // variables on one image unit legal - and both `coherent`, which is what makes the store // through one of them visible to the load through the other. EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform coherent readonly highp image2D " + - StageAlias("goku") + ";")) + RwAlias("goku") + ";")) << out; EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform coherent writeonly highp image2D " + SplitWriteAlias("goku") + ";")) @@ -88,7 +89,7 @@ void main() // The load goes to the readonly half, the store to the writeonly one, and neither is called // what the application called it any more. - EXPECT_TRUE(Contains(out, "imageLoad(" + StageAlias("goku") + ",")); + EXPECT_TRUE(Contains(out, "imageLoad(" + RwAlias("goku") + ",")); EXPECT_TRUE(Contains(out, "imageStore(" + SplitWriteAlias("goku") + ",")); EXPECT_FALSE(Contains(out, "imageStore(goku,")); EXPECT_FALSE(Contains(out, "imageLoad(goku,")); @@ -105,7 +106,7 @@ void main() imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); } )"; - const String out = RemoveLayoutBinding(SplitReadWriteImageUniforms(source, kStage)); + const String out = RemoveLayoutBinding(SplitReadWriteImageUniforms(source)); EXPECT_EQ(CountOf(out, "binding = 5"), 2u); } @@ -120,11 +121,11 @@ void main() mg_FragColor = imageLoad(trunks, ivec3(0)); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba16f) uniform readonly highp image2DArray " + - StageAlias("trunks") + ";")) + RoAlias("trunks") + ";")) << out; - EXPECT_TRUE(Contains(out, "imageLoad(" + StageAlias("trunks") + ",")); + EXPECT_TRUE(Contains(out, "imageLoad(" + RoAlias("trunks") + ",")); EXPECT_FALSE(Contains(out, "writeonly")); EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); EXPECT_EQ(CountOf(out, "image2DArray"), 1u); @@ -138,11 +139,11 @@ void main() imageStore(gohan, ivec2(0), vec4(1.0)); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); EXPECT_TRUE( - Contains(out, "layout(binding = 3, rgba8) uniform writeonly highp image2D " + StageAlias("gohan") + ";")) + Contains(out, "layout(binding = 3, rgba8) uniform writeonly highp image2D " + WoAlias("gohan") + ";")) << out; - EXPECT_TRUE(Contains(out, "imageStore(" + StageAlias("gohan") + ",")); + EXPECT_TRUE(Contains(out, "imageStore(" + WoAlias("gohan") + ",")); EXPECT_FALSE(Contains(out, "readonly")); EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); } @@ -155,7 +156,7 @@ TEST(SplitReadWriteImageUniformsTest, ExemptFormatsAreLeftCompletelyAlone) { const String source = "#version 320 es\nlayout(binding = 4, " + String(format) + ") uniform highp " + type + " vegeta;\nvoid main()\n{\n imageStore(vegeta, ivec2(0), imageLoad(vegeta, " "ivec2(0)));\n}\n"; - EXPECT_EQ(SplitReadWriteImageUniforms(source, kStage), source) << "format " << format; + EXPECT_EQ(SplitReadWriteImageUniforms(source), source) << "format " << format; } } @@ -172,7 +173,7 @@ void main() // Untouched means UNRENAMED too: a declaration that already carries its qualifier in the // source carries the SAME one in every stage, so there is no cross-stage mismatch to break up // and renaming it would only churn the text. - EXPECT_EQ(SplitReadWriteImageUniforms(source, kStage), source); + EXPECT_EQ(SplitReadWriteImageUniforms(source), source); } // The binding of an image array is the array's base; splitting must keep the array on both @@ -185,15 +186,15 @@ void main() imageStore(gohan[1], ivec2(0), imageLoad(gohan[2], ivec2(0))); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform coherent readonly highp image2D " + - StageAlias("gohan") + "[3];")) + RwAlias("gohan") + "[3];")) << out; EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform coherent writeonly highp image2D " + SplitWriteAlias("gohan") + "[3];")) << out; EXPECT_TRUE(Contains(out, "imageStore(" + SplitWriteAlias("gohan") + "[1],")); - EXPECT_TRUE(Contains(out, "imageLoad(" + StageAlias("gohan") + "[2],")); + EXPECT_TRUE(Contains(out, "imageLoad(" + RwAlias("gohan") + "[2],")); } // The rewrite is by identifier, not by substring: "goku" must not reach into "goku_hd", and @@ -209,20 +210,20 @@ void main() imageStore(goku_hd, ivec2(0), loaded); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); // goku is read+write -> split (and coherent with it); goku_hd is write-only -> qualified in // place, not split, and left non-coherent because nothing aliases it. Both are renamed. EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform coherent readonly highp image2D " + - StageAlias("goku") + ";")) + RwAlias("goku") + ";")) << out; EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform coherent writeonly highp image2D " + SplitWriteAlias("goku") + ";")) << out; EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform writeonly highp image2D " + - StageAlias("goku_hd") + ";")) + WoAlias("goku_hd") + ";")) << out; - EXPECT_TRUE(Contains(out, "imageStore(" + StageAlias("goku_hd") + ",")); + EXPECT_TRUE(Contains(out, "imageStore(" + WoAlias("goku_hd") + ",")); EXPECT_FALSE(Contains(out, SplitWriteAlias("goku") + "_hd")); EXPECT_FALSE(Contains(out, SplitWriteAlias("goku_hd"))); } @@ -237,8 +238,8 @@ void main() imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); - EXPECT_TRUE(Contains(out, "uniform readonly coherent restrict highp image2D " + StageAlias("goku") + ";")) + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "uniform readonly coherent restrict highp image2D " + RwAlias("goku") + ";")) << out; EXPECT_TRUE( Contains(out, "uniform writeonly coherent restrict highp image2D " + SplitWriteAlias("goku") + ";")) @@ -266,14 +267,14 @@ void main() imageStore(storeOnly, ivec2(0), vec4(2.0)); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); - EXPECT_TRUE(Contains(out, "uniform coherent readonly highp image2D " + StageAlias("goku") + ";")) << out; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "uniform coherent readonly highp image2D " + RwAlias("goku") + ";")) << out; EXPECT_TRUE(Contains(out, "uniform coherent writeonly highp image2D " + SplitWriteAlias("goku") + ";")) << out; // Exactly the two halves of the pair, and nothing else: the store-only image is repaired in // place, has no alias to stay visible to, and must not pay for uncached access. EXPECT_EQ(CountOf(out, "coherent"), 2u); - EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D " + StageAlias("storeOnly") + ";")) << out; + EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D " + WoAlias("storeOnly") + ";")) << out; } // The ORDERING half of the split, which `coherent` alone does not buy. Coherent makes the store @@ -294,7 +295,7 @@ void main() mg_FragColor = first + imageLoad(goku, ivec2(0)); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); EXPECT_TRUE( Contains(out, "imageStore(" + SplitWriteAlias("goku") + ", ivec2(0), vec4(1.0)); memoryBarrierImage();")) @@ -317,8 +318,8 @@ void main() imageStore(storeOnly, ivec2(0), vec4(1.0)); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); - EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D " + StageAlias("storeOnly") + ";")) << out; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D " + WoAlias("storeOnly") + ";")) << out; EXPECT_FALSE(Contains(out, "memoryBarrierImage")) << out; } @@ -333,8 +334,8 @@ void main() imageStore(gohan[1], ivec2(0), max(imageLoad(gohan[2], ivec2(0)), vec4(0.5))); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); - EXPECT_TRUE(Contains(out, "max(imageLoad(" + StageAlias("gohan") + + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "max(imageLoad(" + RwAlias("gohan") + "[2], ivec2(0)), vec4(0.5))); memoryBarrierImage();")) << out; EXPECT_EQ(CountOf(out, "memoryBarrierImage();"), 1u) << out; @@ -359,7 +360,7 @@ void main() } )"; Uint splitCount = 99u; - SplitReadWriteImageUniforms(twoSplits, kStage, &splitCount); + SplitReadWriteImageUniforms(twoSplits, &splitCount); EXPECT_EQ(splitCount, 2u) << "only the read+write pair counts; the store-only repair adds no uniform"; // Every early return has to write the count too, or a caller reads whatever was there before. @@ -371,7 +372,7 @@ void main() } )"; splitCount = 99u; - SplitReadWriteImageUniforms(noImages, kStage, &splitCount); + SplitReadWriteImageUniforms(noImages, &splitCount); EXPECT_EQ(splitCount, 0u); } @@ -386,20 +387,20 @@ void main() mg_FragColor = vec4(float(imageSize(sizeOnly).x)); } )"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); EXPECT_TRUE(Contains(out, "layout(binding = 8, rgba8ui) uniform readonly highp uimage2D " + - StageAlias("sizeOnly") + ";")) + RoAlias("sizeOnly") + ";")) << out; // The rename has to reach imageSize too, or the declaration and its only use stop agreeing. - EXPECT_TRUE(Contains(out, "imageSize(" + StageAlias("sizeOnly") + ")")) << out; + EXPECT_TRUE(Contains(out, "imageSize(" + RoAlias("sizeOnly") + ")")) << out; EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); } // Neither minted name may land on an identifier the shader already uses - and there are two of -// them now, the stage-tagged name of the repaired declaration and the writeonly half built on +// them now, the access-tagged name of the repaired declaration and the writeonly half built on // top of it. Both collisions are exercised at once. TEST(SplitReadWriteImageUniformsTest, AliasNamesAvoidExistingIdentifiers) { - const String stageCollision = StageAlias("taken"); + const String stageCollision = RwAlias("taken"); const String writeCollision = SplitWriteAlias("taken"); const String source = "#version 320 es\n" "layout(binding = 6, rgba8) uniform highp image2D taken;\n" @@ -408,7 +409,7 @@ TEST(SplitReadWriteImageUniformsTest, AliasNamesAvoidExistingIdentifiers) { ";\nvoid main()\n{\n" " imageStore(taken, ivec2(0), imageLoad(taken, ivec2(0)) + " + stageCollision + " + " + writeCollision + ");\n}\n"; - const String out = SplitReadWriteImageUniforms(source, kStage); + const String out = SplitReadWriteImageUniforms(source); EXPECT_FALSE(Contains(out, "image2D " + stageCollision + ";")) << out; EXPECT_FALSE(Contains(out, "image2D " + writeCollision + ";")) << out; @@ -435,7 +436,7 @@ void main() // Declining means declining EVERYTHING: no qualifier, and no rename either. A rename that // moved the declaration but not the use inside helper() would be a compile error rather than // the wrong-but-compiling shader this pass refuses to guess at. - EXPECT_EQ(SplitReadWriteImageUniforms(source, kStage), source); + EXPECT_EQ(SplitReadWriteImageUniforms(source), source); } TEST(SplitReadWriteImageUniformsTest, ShaderWithoutImagesIsReturnedUnchanged) { @@ -447,7 +448,7 @@ void main() mg_FragColor = texture(goku, vec2(0.5)); } )"; - EXPECT_EQ(SplitReadWriteImageUniforms(source, kStage), source); + EXPECT_EQ(SplitReadWriteImageUniforms(source), source); } // The defect the rename exists for. The pass sees ONE stage at a time and picks the memory @@ -455,9 +456,9 @@ void main() // shader that only loads the same image came out `writeonly g_image` and `readonly g_image` - // two declarations of one uniform name that GLSL requires to be identical. Adreno merges them // and silently discards the vertex-stage stores (advanced-memory-dependentInvocation reads back -// the untouched zeros, with LINK_STATUS = 1 and an empty driver log). Stage-tagged names leave -// nothing to merge. -TEST(SplitReadWriteImageUniformsTest, TheSameImageGetsADifferentNameInEachStage) { +// the untouched zeros, with LINK_STATUS = 1 and an empty driver log). Tagging by the repair +// leaves nothing to merge. +TEST(SplitReadWriteImageUniformsTest, StagesThatUseAnImageDifferentlyGetDifferentNames) { const String vertexSource = R"(#version 320 es layout(binding = 0, rgba32f) uniform coherent highp image2D g_image; void main() @@ -474,11 +475,11 @@ void main() mg_FragColor = imageLoad(g_image, ivec2(0)); } )"; - const String vsOut = SplitReadWriteImageUniforms(vertexSource, GL_VERTEX_SHADER); - const String fsOut = SplitReadWriteImageUniforms(fragmentSource, GL_FRAGMENT_SHADER); + const String vsOut = SplitReadWriteImageUniforms(vertexSource); + const String fsOut = SplitReadWriteImageUniforms(fragmentSource); - const String vsName = ImageStageAliasPrefix(GL_VERTEX_SHADER) + "g_image"; - const String fsName = ImageStageAliasPrefix(GL_FRAGMENT_SHADER) + "g_image"; + const String vsName = WoAlias("g_image"); + const String fsName = RoAlias("g_image"); EXPECT_NE(vsName, fsName); EXPECT_TRUE(Contains(vsOut, "uniform writeonly coherent highp image2D " + vsName + ";")) << vsOut; EXPECT_TRUE(Contains(fsOut, "uniform readonly coherent highp image2D " + fsName + ";")) << fsOut; @@ -493,21 +494,62 @@ void main() EXPECT_TRUE(Contains(fsOut, "binding = 0")); } -// Every stage gets a tag of its own, including the ones a fragment/vertex pair never exercises. -TEST(SplitReadWriteImageUniformsTest, EveryStageTagIsDistinct) { - const GLenum stages[] = {GL_VERTEX_SHADER, GL_FRAGMENT_SHADER, GL_COMPUTE_SHADER, - GL_GEOMETRY_SHADER, GL_TESS_CONTROL_SHADER, GL_TESS_EVALUATION_SHADER}; - Vector prefixes; - for (const GLenum stage : stages) { - const String prefix = ImageStageAliasPrefix(stage); - EXPECT_EQ(prefix.rfind(IMAGE_STAGE_ALIAS_PREFIX, 0), 0u) << prefix; +// The other side of that coin, and the one a per-STAGE tag got wrong. Two stages that use the +// image the same way emit byte-identical declarations, so they must arrive at ONE shared name: +// Adreno allocates an image LOCATION per distinct uniform, and giving each stage its own name +// multiplied a program's image-uniform count by the number of stages that mention it - which is +// how the five stages of KHR-GL43.shading_language_420pack.binding_images_texture_type_* went +// from 6 image uniforms to 30 and drew "Error: Image Image location or component exceeds max +// allowed." out of the Adreno 830 linker, with LINK_STATUS = TRUE already published by the +// frontend and every draw silently doing nothing. +TEST(SplitReadWriteImageUniformsTest, StagesThatUseAnImageAlikeShareOneName) { + const String vertexSource = R"(#version 320 es +layout(binding = 1, rgba8) uniform highp image2D goku; +void main() +{ + imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); + gl_Position = vec4(0.0); +} +)"; + const String fragmentSource = R"(#version 320 es +layout(binding = 1, rgba8) uniform highp image2D goku; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); + mg_FragColor = vec4(0.0); +} +)"; + const String vsOut = SplitReadWriteImageUniforms(vertexSource); + const String fsOut = SplitReadWriteImageUniforms(fragmentSource); + + // One name, arrived at independently by two different stages, so the linker merges them + // back into the single image uniform the application declared. + for (const String& out : {vsOut, fsOut}) { + EXPECT_TRUE(Contains(out, "uniform coherent readonly highp image2D " + RwAlias("goku") + ";")) << out; + EXPECT_TRUE(Contains(out, "uniform coherent writeonly highp image2D " + SplitWriteAlias("goku") + ";")) + << out; + EXPECT_TRUE(Contains(out, "imageLoad(" + RwAlias("goku") + ",")) << out; + EXPECT_TRUE(Contains(out, "imageStore(" + SplitWriteAlias("goku") + ",")) << out; + } +} + +// One tag per repair, all three distinct, and each a legal identifier stem. +TEST(SplitReadWriteImageUniformsTest, EveryAccessTagIsDistinct) { + const String prefixes[] = {String(IMAGE_READONLY_ALIAS_PREFIX), String(IMAGE_WRITEONLY_ALIAS_PREFIX), + String(IMAGE_SPLIT_READ_ALIAS_PREFIX), String(IMAGE_WRITE_ALIAS_PREFIX)}; + Vector seenPrefixes; + for (const String& prefix : prefixes) { // A GLSL identifier may not contain "__" (GLSL ES 3.20 3.7), and the prefix is glued // straight onto a name that may itself start with '_'. EXPECT_EQ(prefix.find("__"), String::npos) << prefix; - for (const String& seen : prefixes) { + for (const String& seen : seenPrefixes) { EXPECT_NE(seen, prefix) << prefix; + // Nor may one be a prefix of another: the write half is minted on top of an + // already-tagged name, so a shared stem would let two repairs collide. + EXPECT_NE(prefix.rfind(seen, 0), 0u) << prefix << " vs " << seen; } - prefixes.push_back(prefix); + seenPrefixes.push_back(prefix); } } @@ -690,17 +732,17 @@ void main() } )"; String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 6})}); - out = SplitReadWriteImageUniforms(out, kStage); + out = SplitReadWriteImageUniforms(out); out = RemoveLayoutBinding(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)))")) + EXPECT_TRUE(Contains(out, "readonly highp image2D " + RoAlias(Elem("g_image", 0)) + ";")) << out; + EXPECT_TRUE(Contains(out, "writeonly highp image2D " + WoAlias(Elem("g_image", 1)) + ";")) << out; + EXPECT_TRUE(Contains(out, "imageStore(" + WoAlias(Elem("g_image", 1)) + ", ivec2(0), imageLoad(" + + RoAlias(Elem("g_image", 0)) + ", ivec2(0)))")) << out; // Nothing is left addressing the array. EXPECT_FALSE(Contains(out, "g_image[")) << out; @@ -718,17 +760,54 @@ void main() } )"; String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 9})}); - out = SplitReadWriteImageUniforms(out, kStage); + out = SplitReadWriteImageUniforms(out); 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))) + ";")) + EXPECT_TRUE(Contains(out, "readonly highp image2D " + RwAlias(Elem("g_image", 1)) + ";")) << out; + EXPECT_TRUE(Contains(out, "writeonly highp image2D " + WriteAlias(RwAlias(Elem("g_image", 1))) + ";")) << out; EXPECT_EQ(CountOf(out, "binding = 4"), 1u) << out; } +// The gap that let a per-STAGE image rename reach production: every fixture above declares an +// image ARRAY, and the regression it caused was in the SCALAR images sitting next to one. A +// scalar with an explicit binding has to come out of the whole chain still on ITS OWN unit, +// still spelled once, and named the same thing every stage would name it - it is the array that +// needs repairing, not its neighbour. +TEST(RemapImageArrayElementUnitsTest, AScalarImageWithItsOwnBindingIsUntouchedByTheArrayRepair) { + const String source = R"(#version 320 es +layout(rgba8, binding = 7) uniform highp image2D goku; +layout(rgba32f, binding = 4) uniform highp image2D g_image[2]; +void main() +{ + imageStore(g_image[1], ivec2(0), imageLoad(g_image[0], ivec2(0))); + imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); +} +)"; + Vector declined; + String out = RemapImageArrayElementUnits(source, {Plan("g_image", {4, 9})}, &declined); + EXPECT_TRUE(declined.empty()); + // The array pass may only ever touch the arrays it was handed a plan for. + EXPECT_TRUE(Contains(out, "layout(rgba8, binding = 7) uniform highp image2D goku;")) << out; + + out = SplitReadWriteImageUniforms(out); + out = RemoveLayoutBinding(out); + + // Unit 7 exactly twice - the two halves of the scalar's own split pair - and nothing has + // moved it onto one of the array's units. + EXPECT_EQ(CountOf(out, "binding = 7"), 2u) << out; + EXPECT_TRUE(Contains(out, "readonly highp image2D " + RwAlias("goku") + ";")) << out; + EXPECT_TRUE(Contains(out, "writeonly highp image2D " + SplitWriteAlias("goku") + ";")) << out; + // ...and no per-stage tag anywhere: the name a scalar gets is a function of how this text + // uses it, so every stage that uses it the same way keeps ONE shared uniform (Adreno spends + // an image location per distinct one). + EXPECT_FALSE(Contains(out, "mg_imageVs_")) << out; + EXPECT_FALSE(Contains(out, "mg_imageFs_")) << out; + EXPECT_FALSE(Contains(out, "mg_imageCs_")) << out; +} + // --------------------------------------------------------------------------------------- // RetargetTextureBufferExtension // @@ -971,7 +1050,7 @@ layout(binding = 3) uniform writeonly highp uimage2D uni_image; void main() { imageStore(uni_image, ivec2(0), uvec4(1u)); } )"; String out = BakeImageFormatQualifiers(source, {{"uni_image", "r8ui"}}); - out = SplitReadWriteImageUniforms(out, kStage); + out = SplitReadWriteImageUniforms(out); out = RemoveLayoutBinding(out); EXPECT_TRUE(Contains(out, "r8ui")) << out; EXPECT_TRUE(Contains(out, "binding = 3")) << out;