diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index cea0df7d..32d17b19 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -6192,8 +6192,11 @@ 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. Uint splitImageUniformCount = 0; - source = SplitReadWriteImageUniforms(source, &splitImageUniformCount); + source = SplitReadWriteImageUniforms(source, glShaderType, &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 be295362..8b18c29e 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -857,6 +857,8 @@ namespace MobileGL::MG_Backend::DirectGLES { struct ImageUniformDecl { String name; + String stageName; // the stage-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(...) String qualifiers; // memory/precision qualifiers, normalized, no trailing space @@ -901,11 +903,11 @@ namespace MobileGL::MG_Backend::DirectGLES { return out; } - // A name for the writeonly half that no identifier in the shader (and no other - // half already minted) can collide with. - String MakeImageWriteAliasName(const String& name, const String& source, - const Vector& taken) { - String candidate = String(IMAGE_WRITE_ALIAS_PREFIX) + name; + // A name for a rewritten declaration that no identifier in the shader (and no other + // alias already minted for this stage) can collide with. + String MakeImageAliasName(const String& prefix, const String& name, const String& source, + const Vector& taken) { + String candidate = prefix + name; // "__" anywhere in an identifier is reserved (GLSL ES 3.20 3.7), which a name // that already starts with '_' would otherwise produce. for (SizeT doubled = candidate.find("__"); doubled != String::npos; @@ -951,7 +953,22 @@ namespace MobileGL::MG_Backend::DirectGLES { } } // namespace - String SplitReadWriteImageUniforms(const String& glslCode, Uint* outSplitCount) { + 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_"; + } + } + + String SplitReadWriteImageUniforms(const String& glslCode, GLenum shaderType, Uint* outSplitCount) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif @@ -1014,13 +1031,17 @@ namespace MobileGL::MG_Backend::DirectGLES { }; // Walk every `image*(` call and attribute its first argument to a declaration. - struct StoreSite { + // EVERY recognized use is recorded, not only the stores: a declaration this pass + // renames has to take all of its uses with it, and the "every occurrence was one I + // saw" check below is what makes the recorded set provably the complete set. + struct ImageUseSite { SizeT declIndex; SizeT start; SizeT length; SizeT callOpen; // the '(' of the call this argument belongs to + Bool stores; // an imageStore, i.e. the use a split redirects to the write half }; - Vector storeSites; + Vector useSites; for (SizeT pos = glslCode.find("image"); pos != String::npos; pos = glslCode.find("image", pos + 1)) { if (pos > 0 && IsImagePassIdentifierChar(glslCode[pos - 1])) continue; // uimage2D, myimageFoo SizeT tokenEnd = pos; @@ -1065,12 +1086,16 @@ namespace MobileGL::MG_Backend::DirectGLES { switch (ClassifyImageBuiltin(builtin)) { case ImageBuiltinAccess::Load: decl.loaded = true; + useSites.push_back({declIndex, argStart, argEnd - argStart, openParen, false}); break; case ImageBuiltinAccess::Store: decl.stored = true; - storeSites.push_back({declIndex, argStart, argEnd - argStart, openParen}); + useSites.push_back({declIndex, argStart, argEnd - argStart, openParen, true}); break; case ImageBuiltinAccess::None: + // imageSize/imageSamples touch nothing, but they still NAME the variable, so + // a rename has to reach them. + useSites.push_back({declIndex, argStart, argEnd - argStart, openParen, false}); break; default: decl.unknownUse = true; @@ -1087,12 +1112,40 @@ namespace MobileGL::MG_Backend::DirectGLES { } Vector edits; - Vector takenAliases; + 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 + // (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. + // + // 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); if (decl.loaded && decl.stored) { - decl.writeName = MakeImageWriteAliasName(decl.name, glslCode, takenAliases); - takenAliases.push_back(decl.writeName); + // Minted from the ALREADY stage-tagged name, so two stages that both split + // the same image do not collide on the write half either. + decl.writeName = + MakeImageAliasName(IMAGE_WRITE_ALIAS_PREFIX, decl.stageName, glslCode, takenNames); + takenNames.push_back(decl.writeName); decl.split = true; if (outSplitCount != nullptr) ++*outSplitCount; // Both halves carry `coherent`; see BuildImageDeclaration. The @@ -1100,24 +1153,29 @@ 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.name, /*forceCoherent=*/true) + + BuildImageDeclaration(decl, "readonly", decl.stageName, + /*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.name)}); + BuildImageDeclaration(decl, "writeonly", decl.stageName)}); } 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.name)}); + BuildImageDeclaration(decl, "readonly", decl.stageName)}); } } - for (const StoreSite& site : storeSites) { + for (const ImageUseSite& site : useSites) { const ImageUniformDecl& decl = decls[site.declIndex]; - if (!decl.split) continue; - edits.push_back({site.start, site.length, decl.writeName}); + // 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; + edits.push_back( + {site.start, site.length, decl.split && site.stores ? decl.writeName : decl.stageName}); + 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 // about ORDER within one invocation - and the whole reason a declaration is split diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 0bae6e95..067827b7 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -236,8 +236,15 @@ namespace MobileGL::MG_Backend::DirectGLES { String BakeImageFormatQualifiers(String glslCode, const UnorderedMap& esslFormatByUniformName); String RemoveLayoutBinding(const String& glslCode); // Prefix of the writeonly half a read+write image uniform is split into (see - // SplitReadWriteImageUniforms); the suffix is the image's own name. + // SplitReadWriteImageUniforms); the suffix is the image's own (already stage-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); // 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). @@ -249,7 +256,7 @@ 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: + // Restores a legal declaration, and RENAMES it per stage while doing so: // * loaded only -> add `readonly` // * stored only -> add `writeonly` // * both -> emit TWO declarations on the same binding and of the @@ -261,6 +268,20 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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 + // (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. + // // 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 // one when both are coherent, and the split is what makes a same-variable @@ -284,13 +305,16 @@ 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. + // 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. // // `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, Uint* outSplitCount = nullptr); + String SplitReadWriteImageUniforms(const String& glslCode, GLenum shaderType, + 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 91ead160..5d707419 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -18,7 +18,9 @@ using namespace MobileGL; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::BakeImageFormatQualifiers; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ForceFlatIntegerVaryings; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_STAGE_ALIAS_PREFIX; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITE_ALIAS_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ImageStageAliasPrefix; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemoveLayoutBinding; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestExtendedImageFormats; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RequestViewportArrayExtension; @@ -37,7 +39,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. String WriteAlias(const String& name) { return String(IMAGE_WRITE_ALIAS_PREFIX) + name; } + String SplitWriteAlias(const String& name) { return WriteAlias(StageAlias(name)); } } // namespace // The bug the pass exists for. SPIRV-Cross speculatively marks every storage image @@ -56,19 +65,24 @@ void main() mg_FragColor = loaded; } )"; - const String out = SplitReadWriteImageUniforms(source); + const String out = SplitReadWriteImageUniforms(source, kStage); // 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 goku;")); - EXPECT_TRUE(Contains( - out, "layout(binding = 2, rgba8) uniform coherent writeonly highp image2D " + WriteAlias("goku") + ";")); + EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform coherent readonly highp image2D " + + StageAlias("goku") + ";")) + << out; + EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform coherent writeonly highp image2D " + + SplitWriteAlias("goku") + ";")) + << out; - // The load keeps the original name, the store moves to the writeonly half. - EXPECT_TRUE(Contains(out, "imageLoad(goku,")); - EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ",")); + // 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, "imageStore(" + SplitWriteAlias("goku") + ",")); EXPECT_FALSE(Contains(out, "imageStore(goku,")); + EXPECT_FALSE(Contains(out, "imageLoad(goku,")); } // The split has to survive RemoveLayoutBinding, which runs straight after it: an ES image @@ -82,7 +96,7 @@ void main() imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); } )"; - const String out = RemoveLayoutBinding(SplitReadWriteImageUniforms(source)); + const String out = RemoveLayoutBinding(SplitReadWriteImageUniforms(source, kStage)); EXPECT_EQ(CountOf(out, "binding = 5"), 2u); } @@ -97,8 +111,11 @@ void main() mg_FragColor = imageLoad(trunks, ivec3(0)); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba16f) uniform readonly highp image2DArray trunks;")); + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba16f) uniform readonly highp image2DArray " + + StageAlias("trunks") + ";")) + << out; + EXPECT_TRUE(Contains(out, "imageLoad(" + StageAlias("trunks") + ",")); EXPECT_FALSE(Contains(out, "writeonly")); EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); EXPECT_EQ(CountOf(out, "image2DArray"), 1u); @@ -112,8 +129,11 @@ void main() imageStore(gohan, ivec2(0), vec4(1.0)); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "layout(binding = 3, rgba8) uniform writeonly highp image2D gohan;")); + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE( + Contains(out, "layout(binding = 3, rgba8) uniform writeonly highp image2D " + StageAlias("gohan") + ";")) + << out; + EXPECT_TRUE(Contains(out, "imageStore(" + StageAlias("gohan") + ",")); EXPECT_FALSE(Contains(out, "readonly")); EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); } @@ -126,7 +146,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), source) << "format " << format; + EXPECT_EQ(SplitReadWriteImageUniforms(source, kStage), source) << "format " << format; } } @@ -140,7 +160,10 @@ void main() imageStore(writer, ivec2(0), imageLoad(reader, ivec2(0))); } )"; - EXPECT_EQ(SplitReadWriteImageUniforms(source), source); + // 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); } // The binding of an image array is the array's base; splitting must keep the array on both @@ -153,12 +176,15 @@ void main() imageStore(gohan[1], ivec2(0), imageLoad(gohan[2], ivec2(0))); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform coherent readonly highp image2D gohan[3];")); - EXPECT_TRUE(Contains( - out, "layout(binding = 6, rgba8) uniform coherent writeonly highp image2D " + WriteAlias("gohan") + "[3];")); - EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("gohan") + "[1],")); - EXPECT_TRUE(Contains(out, "imageLoad(gohan[2],")); + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform coherent readonly highp image2D " + + StageAlias("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],")); } // The rewrite is by identifier, not by substring: "goku" must not reach into "goku_hd", and @@ -174,17 +200,22 @@ void main() imageStore(goku_hd, ivec2(0), loaded); } )"; - const String out = SplitReadWriteImageUniforms(source); + const String out = SplitReadWriteImageUniforms(source, kStage); // 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. - EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform coherent readonly highp image2D goku;")); - EXPECT_TRUE(Contains( - out, "layout(binding = 1, rgba8) uniform coherent writeonly highp image2D " + WriteAlias("goku") + ";")); - EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform writeonly highp image2D goku_hd;")); - EXPECT_TRUE(Contains(out, "imageStore(goku_hd,")); - EXPECT_FALSE(Contains(out, WriteAlias("goku") + "_hd")); - EXPECT_FALSE(Contains(out, WriteAlias("goku_hd"))); + // 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") + ";")) + << 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") + ";")) + << out; + EXPECT_TRUE(Contains(out, "imageStore(" + StageAlias("goku_hd") + ",")); + EXPECT_FALSE(Contains(out, SplitWriteAlias("goku") + "_hd")); + EXPECT_FALSE(Contains(out, SplitWriteAlias("goku_hd"))); } // Other qualifiers belong to both halves, and the memory qualifier goes where SPIRV-Cross @@ -197,10 +228,12 @@ void main() imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "uniform readonly coherent restrict highp image2D goku;")); + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "uniform readonly coherent restrict highp image2D " + StageAlias("goku") + ";")) + << out; EXPECT_TRUE( - Contains(out, "uniform writeonly coherent restrict highp image2D " + WriteAlias("goku") + ";")); + Contains(out, "uniform writeonly coherent restrict highp image2D " + SplitWriteAlias("goku") + ";")) + << out; // ...and the coherent the split adds is not a SECOND one: a repeated memory qualifier is a // compile error in ESSL, so the source's own has to be recognized. EXPECT_EQ(CountOf(out, "coherent"), 2u); @@ -224,13 +257,14 @@ void main() imageStore(storeOnly, ivec2(0), vec4(2.0)); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "uniform coherent readonly highp image2D goku;")) << out; - EXPECT_TRUE(Contains(out, "uniform coherent writeonly highp image2D " + WriteAlias("goku") + ";")) << out; + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "uniform coherent readonly highp image2D " + StageAlias("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 storeOnly;")) << out; + EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D " + StageAlias("storeOnly") + ";")) << out; } // The ORDERING half of the split, which `coherent` alone does not buy. Coherent makes the store @@ -251,11 +285,13 @@ void main() mg_FragColor = first + imageLoad(goku, ivec2(0)); } )"; - const String out = SplitReadWriteImageUniforms(source); + const String out = SplitReadWriteImageUniforms(source, kStage); - EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ", ivec2(0), vec4(1.0)); memoryBarrierImage();")) + EXPECT_TRUE( + Contains(out, "imageStore(" + SplitWriteAlias("goku") + ", ivec2(0), vec4(1.0)); memoryBarrierImage();")) << out; - EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ", ivec2(0), vec4(2.0)); memoryBarrierImage();")) + EXPECT_TRUE( + Contains(out, "imageStore(" + SplitWriteAlias("goku") + ", ivec2(0), vec4(2.0)); memoryBarrierImage();")) << out; // One per store, not one per shader and not one per load. EXPECT_EQ(CountOf(out, "memoryBarrierImage();"), 2u) << out; @@ -272,8 +308,8 @@ void main() imageStore(storeOnly, ivec2(0), vec4(1.0)); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D storeOnly;")) << out; + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D " + StageAlias("storeOnly") + ";")) << out; EXPECT_FALSE(Contains(out, "memoryBarrierImage")) << out; } @@ -288,8 +324,10 @@ void main() imageStore(gohan[1], ivec2(0), max(imageLoad(gohan[2], ivec2(0)), vec4(0.5))); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "max(imageLoad(gohan[2], ivec2(0)), vec4(0.5))); memoryBarrierImage();")) << out; + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "max(imageLoad(" + StageAlias("gohan") + + "[2], ivec2(0)), vec4(0.5))); memoryBarrierImage();")) + << out; EXPECT_EQ(CountOf(out, "memoryBarrierImage();"), 1u) << out; } @@ -312,7 +350,7 @@ void main() } )"; Uint splitCount = 99u; - SplitReadWriteImageUniforms(twoSplits, &splitCount); + SplitReadWriteImageUniforms(twoSplits, kStage, &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. @@ -324,7 +362,7 @@ void main() } )"; splitCount = 99u; - SplitReadWriteImageUniforms(noImages, &splitCount); + SplitReadWriteImageUniforms(noImages, kStage, &splitCount); EXPECT_EQ(splitCount, 0u); } @@ -339,26 +377,39 @@ void main() mg_FragColor = vec4(float(imageSize(sizeOnly).x)); } )"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_TRUE(Contains(out, "layout(binding = 8, rgba8ui) uniform readonly highp uimage2D sizeOnly;")); + const String out = SplitReadWriteImageUniforms(source, kStage); + EXPECT_TRUE(Contains(out, "layout(binding = 8, rgba8ui) uniform readonly highp uimage2D " + + StageAlias("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_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); } -// The alias must not land on an identifier the shader already uses. -TEST(SplitReadWriteImageUniformsTest, AliasNameAvoidsAnExistingIdentifier) { - const String source = R"(#version 320 es -layout(binding = 6, rgba8) uniform highp image2D taken; -highp vec4 mg_imageWrite_taken; -void main() -{ - imageStore(taken, ivec2(0), imageLoad(taken, ivec2(0)) + mg_imageWrite_taken); -} -)"; - const String out = SplitReadWriteImageUniforms(source); - EXPECT_FALSE(Contains(out, "image2D " + WriteAlias("taken") + ";")); - EXPECT_TRUE(Contains(out, "image2D " + WriteAlias("taken") + "X;")); - EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("taken") + "X,")); - EXPECT_TRUE(Contains(out, "+ mg_imageWrite_taken)")); +// 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 +// top of it. Both collisions are exercised at once. +TEST(SplitReadWriteImageUniformsTest, AliasNamesAvoidExistingIdentifiers) { + const String stageCollision = StageAlias("taken"); + const String writeCollision = SplitWriteAlias("taken"); + const String source = "#version 320 es\n" + "layout(binding = 6, rgba8) uniform highp image2D taken;\n" + "highp vec4 " + + stageCollision + ";\nhighp vec4 " + writeCollision + + ";\nvoid main()\n{\n" + " imageStore(taken, ivec2(0), imageLoad(taken, ivec2(0)) + " + + stageCollision + " + " + writeCollision + ");\n}\n"; + const String out = SplitReadWriteImageUniforms(source, kStage); + + EXPECT_FALSE(Contains(out, "image2D " + stageCollision + ";")) << out; + EXPECT_FALSE(Contains(out, "image2D " + writeCollision + ";")) << out; + EXPECT_TRUE(Contains(out, "image2D " + stageCollision + "X;")) << out; + EXPECT_TRUE(Contains(out, "image2D " + writeCollision + "X;")) << out; + EXPECT_TRUE(Contains(out, "imageStore(" + writeCollision + "X,")) << out; + EXPECT_TRUE(Contains(out, "imageLoad(" + stageCollision + "X,")) << out; + // ...and the globals that forced the suffix are still themselves. + EXPECT_TRUE(Contains(out, "highp vec4 " + stageCollision + ";")) << out; + EXPECT_TRUE(Contains(out, "highp vec4 " + writeCollision + ";")) << out; } // A use the pass cannot account for (here: the image handed to a user function) means it @@ -372,7 +423,10 @@ void main() imageStore(passed, ivec2(0), helper(passed)); } )"; - EXPECT_EQ(SplitReadWriteImageUniforms(source), source); + // 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); } TEST(SplitReadWriteImageUniformsTest, ShaderWithoutImagesIsReturnedUnchanged) { @@ -384,7 +438,68 @@ void main() mg_FragColor = texture(goku, vec2(0.5)); } )"; - EXPECT_EQ(SplitReadWriteImageUniforms(source), source); + EXPECT_EQ(SplitReadWriteImageUniforms(source, kStage), source); +} + +// The defect the rename exists for. The pass sees ONE stage at a time and picks the memory +// qualifier from the accesses in THAT stage, so a vertex shader that only stores and a fragment +// 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) { + const String vertexSource = R"(#version 320 es +layout(binding = 0, rgba32f) uniform coherent highp image2D g_image; +void main() +{ + imageStore(g_image, ivec2(0), vec4(1.0)); + gl_Position = vec4(0.0); +} +)"; + const String fragmentSource = R"(#version 320 es +layout(binding = 0, rgba32f) uniform coherent highp image2D g_image; +layout(location = 0) out highp vec4 mg_FragColor; +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 vsName = ImageStageAliasPrefix(GL_VERTEX_SHADER) + "g_image"; + const String fsName = ImageStageAliasPrefix(GL_FRAGMENT_SHADER) + "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; + EXPECT_TRUE(Contains(vsOut, "imageStore(" + vsName + ",")) << vsOut; + EXPECT_TRUE(Contains(fsOut, "imageLoad(" + fsName + ",")) << fsOut; + // The whole point: after the rewrite the two stages no longer declare a common name, so + // there is nothing for a linker to merge and mis-qualify. + EXPECT_FALSE(Contains(vsOut, fsName)) << vsOut; + EXPECT_FALSE(Contains(fsOut, vsName)) << fsOut; + // Both bindings are untouched - the image unit is still the same one. + EXPECT_TRUE(Contains(vsOut, "binding = 0")); + 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; + // 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) { + EXPECT_NE(seen, prefix) << prefix; + } + prefixes.push_back(prefix); + } } // --------------------------------------------------------------------------------------- @@ -629,7 +744,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); + out = SplitReadWriteImageUniforms(out, kStage); out = RemoveLayoutBinding(out); EXPECT_TRUE(Contains(out, "r8ui")) << out; EXPECT_TRUE(Contains(out, "binding = 3")) << out;