From 8f19ce6fa78b75bd8631c038d0f822eaa5b9719b Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 22 Aug 2026 03:46:35 -0400 Subject: [PATCH] [Fix, Test] (DirectGLES): rename an image SPIRV-Cross already qualified so two stages cannot merge it --- MobileGL/MG_Backend/DirectGLES/Utils.cpp | 50 ++++++++-- MobileGL/MG_Backend/DirectGLES/Utils.h | 16 ++- .../Backend/DirectGLES/EsslShaderPassTest.cpp | 99 ++++++++++++++++++- 3 files changed, 150 insertions(+), 15 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index 2d3ba4c1..12006eda 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -955,11 +955,17 @@ namespace MobileGL::MG_Backend::DirectGLES { String arraySuffix; // "" or "[7]" SizeT declStart = 0; SizeT declLength = 0; + SizeT nameStart = 0; // the name token alone, for a rename that edits nothing else + SizeT nameLength = 0; SizeT referenceCount = 0; // uses this pass recognized and accounted for Bool loaded = false; Bool stored = false; Bool unknownUse = false; Bool split = false; + // SPIRV-Cross already tagged this one readonly or writeonly, so it needs no + // qualifier repair - only the rename that keeps two stages from merging it. + Bool preTaggedReadonly = false; + Bool preTaggedWriteonly = false; }; // A rebuilt declaration. Keeps SPIRV-Cross's own word order (`uniform readonly @@ -1296,10 +1302,12 @@ namespace MobileGL::MG_Backend::DirectGLES { for (std::sregex_iterator it(glslCode.begin(), glslCode.end(), imageDeclRegex), last; it != last; ++it) { const std::smatch& match = *it; const String qualifiers = match[2].str(); - // Already legal: SPIRV-Cross decided one way, leave it alone. - if (ContainsIdentifier(qualifiers, "readonly") || ContainsIdentifier(qualifiers, "writeonly")) { - continue; - } + const Bool hasReadonly = ContainsIdentifier(qualifiers, "readonly"); + const Bool hasWriteonly = ContainsIdentifier(qualifiers, "writeonly"); + // Carrying BOTH is a spelling no per-stage access analysis produces (SPIRV-Cross + // clears one decoration or the other as soon as it sees a load or a store), so it + // came from the application and is identical in every stage. Nothing to do. + if (hasReadonly && hasWriteonly) continue; Bool hasFormat = false; Bool exemptFormat = false; @@ -1308,10 +1316,12 @@ namespace MobileGL::MG_Backend::DirectGLES { hasFormat = true; exemptFormat = IsMemoryQualifierExemptImageFormat(token); } - // No format qualifier at all is a different (and, in ES, unconditionally - // illegal) shape that GL_EXT_shader_image_load_formatted would be needed for; - // SPIRV-Cross refuses to emit it for an ES target, so nothing to do here. - if (!hasFormat || exemptFormat) continue; + // A declaration carrying neither qualifier is illegal ES unless its format is + // r32f/r32i/r32ui, and no format qualifier at all is a shape SPIRV-Cross refuses + // to emit for an ES target. Either way there is no repair to make - and no rename + // to make either, because a declaration with no access qualifier is spelled the + // same in every stage. + if (!hasReadonly && !hasWriteonly && (!hasFormat || exemptFormat)) continue; ImageUniformDecl decl; decl.layout = match[1].str(); @@ -1321,6 +1331,10 @@ namespace MobileGL::MG_Backend::DirectGLES { decl.arraySuffix = NormalizeDeclarationSpacing(match[5].str()); decl.declStart = static_cast(match.position(0)); decl.declLength = match[0].str().size(); + decl.nameStart = static_cast(match.position(4)); + decl.nameLength = match[4].str().size(); + decl.preTaggedReadonly = hasReadonly; + decl.preTaggedWriteonly = hasWriteonly; decls.push_back(Move(decl)); } if (decls.empty()) { @@ -1452,6 +1466,26 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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. + if (decl.preTaggedReadonly || decl.preTaggedWriteonly) { + // No repair: SPIRV-Cross already emitted a legal qualifier. But it derived + // that qualifier from THIS STAGE's accesses, so a uniform stored in one stage + // and loaded in another arrives here `writeonly` in one and `readonly` in the + // other under ONE name - precisely the same-name/mismatched-qualifier pair + // Adreno merges while silently discarding the writing stage's stores + // (advanced-memory-dependentInvocation; a raw-ES probe reproduces it with no + // MobileGL in the process, and renaming either half fixes it). Keyed on the + // qualifier for the same reason the repair below is: two stages that agree + // spell the same alias and stay merged, so no shader gains an image uniform. + const char* preTagPrefix = + decl.preTaggedReadonly ? IMAGE_READONLY_ALIAS_PREFIX : IMAGE_WRITEONLY_ALIAS_PREFIX; + decl.aliasName = MakeImageAliasName(preTagPrefix, decl.name, glslCode, takenNames); + takenNames.push_back(decl.aliasName); + // The name token alone: the qualifiers are already right, and re-emitting the + // whole declaration would only risk changing them. + edits.push_back({decl.nameStart, decl.nameLength, decl.aliasName}); + continue; + } + const char* aliasPrefix = decl.loaded && decl.stored ? IMAGE_SPLIT_READ_ALIAS_PREFIX : decl.stored ? IMAGE_WRITEONLY_ALIAS_PREFIX : IMAGE_READONLY_ALIAS_PREFIX; diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index b062671c..bc9e96d1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -399,8 +399,20 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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. + // A declaration SPIRV-Cross already tagged `readonly` or `writeonly` needs no qualifier + // repair, but it is NOT stage-independent: that tag is derived from the accesses of the + // stage being emitted, so an image stored in the vertex stage and loaded in the fragment + // stage arrives here as `coherent writeonly g_image` and `coherent readonly g_image` - + // one name, two spellings, which is exactly the pair Adreno merges. Those declarations + // are therefore renamed too, keyed on the qualifier they already carry (readonly -> + // IMAGE_READONLY_ALIAS_PREFIX, writeonly -> IMAGE_WRITEONLY_ALIAS_PREFIX) and with + // nothing but the identifier changed. Stages that agree still reach the same alias and + // stay merged, so this costs no shader an extra image uniform. + // + // The declarations this pass still leaves untouched keep their names: one carrying BOTH + // readonly and writeonly (a spelling no access analysis produces, so it came from the + // application and is identical everywhere), and one carrying NEITHER, which is legal only + // for the r32f/r32i/r32ui formats and is likewise spelled the same in every stage. // // 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 diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 831532e1..e45088ba 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -160,8 +160,12 @@ TEST(SplitReadWriteImageUniformsTest, ExemptFormatsAreLeftCompletelyAlone) { } } -// A declaration SPIRV-Cross already qualified is none of this pass's business. -TEST(SplitReadWriteImageUniformsTest, AlreadyQualifiedDeclarationsAreUntouched) { +// A declaration SPIRV-Cross already qualified needs no REPAIR - but it still needs the rename. +// The input to this pass is SPIRV-Cross output, not application source, and SPIRV-Cross picks +// `readonly` or `writeonly` from the accesses of the stage it is emitting, so "already qualified" +// says nothing about whether the other stages spell it the same way. The qualifiers must survive +// untouched; only the identifier changes. +TEST(SplitReadWriteImageUniformsTest, AlreadyQualifiedDeclarationsAreRenamedButNotRequalified) { const String source = R"(#version 320 es layout(binding = 0, rgba8) uniform readonly highp image2D reader; layout(binding = 1, rgba8) uniform writeonly highp image2D writer; @@ -170,9 +174,33 @@ void main() imageStore(writer, ivec2(0), imageLoad(reader, ivec2(0))); } )"; - // 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. + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "layout(binding = 0, rgba8) uniform readonly highp image2D " + + RoAlias("reader") + ";")) + << out; + EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform writeonly highp image2D " + + WoAlias("writer") + ";")) + << out; + EXPECT_TRUE(Contains(out, "imageStore(" + WoAlias("writer") + ",")) << out; + EXPECT_TRUE(Contains(out, "imageLoad(" + RoAlias("reader") + ",")) << out; + // Neither declaration is doubled and neither gains a qualifier it did not have: this is a + // rename, not a repair. + EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)) << out; + EXPECT_EQ(CountOf(out, "coherent"), 0u) << out; + EXPECT_FALSE(Contains(out, "memoryBarrierImage")) << out; +} + +// A declaration carrying BOTH qualifiers is a spelling no per-stage access analysis produces, so +// it came from the application and reads the same in every stage. Nothing to rename. +TEST(SplitReadWriteImageUniformsTest, ADeclarationQualifiedBothWaysIsLeftCompletelyAlone) { + const String source = R"(#version 320 es +layout(binding = 0, rgba8) uniform readonly writeonly highp image2D inert; +void main() +{ + highp ivec2 size = imageSize(inert); + if (size.x < 0) discard; +} +)"; EXPECT_EQ(SplitReadWriteImageUniforms(source), source); } @@ -494,6 +522,67 @@ void main() EXPECT_TRUE(Contains(fsOut, "binding = 0")); } +// The same defect, in the shape it actually reaches the driver in. SPIRV-Cross emits the access +// qualifier ITSELF whenever the stage only loads or only stores, so the declaration arrives here +// already legal - and this pass used to skip it on exactly that ground, leaving the vertex stage's +// `coherent writeonly g_image` and the fragment stage's `coherent readonly g_image` sharing one +// name. That is the pair a raw-ES probe on the Adreno 830 reproduces with no MobileGL in the +// process: the fragment stage reads back the untouched zeros +// (KHR-GL4x.shader_image_load_store.advanced-memory-dependentInvocation's [1,0,0,0.2]), and +// renaming either half fixes it. This is the emitted text of that test, verbatim. +TEST(SplitReadWriteImageUniformsTest, StagesSpirvCrossQualifiedDifferentlyGetDifferentNames) { + const String vertexSource = R"(#version 320 es +layout(binding = 1, rgba32f) uniform coherent writeonly highp image2D g_image; +void main() +{ + imageStore(g_image, ivec2(0), vec4(2.0)); + gl_Position = vec4(0.0); +} +)"; + const String fragmentSource = R"(#version 320 es +layout(binding = 1, rgba32f) uniform coherent readonly 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); + const String fsOut = SplitReadWriteImageUniforms(fragmentSource); + + const String vsName = WoAlias("g_image"); + const String fsName = RoAlias("g_image"); + EXPECT_NE(vsName, fsName); + EXPECT_TRUE(Contains(vsOut, "uniform coherent writeonly highp image2D " + vsName + ";")) << vsOut; + EXPECT_TRUE(Contains(fsOut, "uniform coherent readonly highp image2D " + fsName + ";")) << fsOut; + EXPECT_TRUE(Contains(vsOut, "imageStore(" + vsName + ",")) << vsOut; + EXPECT_TRUE(Contains(fsOut, "imageLoad(" + fsName + ",")) << fsOut; + // Nothing left for a linker to merge and mis-qualify... + EXPECT_FALSE(Contains(vsOut, fsName)) << vsOut; + EXPECT_FALSE(Contains(fsOut, vsName)) << fsOut; + // ...and the image unit is still the one the application asked for. + EXPECT_TRUE(Contains(vsOut, "binding = 1")) << vsOut; + EXPECT_TRUE(Contains(fsOut, "binding = 1")) << fsOut; +} + +// ...and the budget half of it: two stages SPIRV-Cross qualified the SAME way must still land on +// one shared name, or every stage that names the image spends an image location of its own. +TEST(SplitReadWriteImageUniformsTest, StagesSpirvCrossQualifiedAlikeShareOneName) { + const String stage = R"(#version 320 es +layout(binding = 1, rgba32f) uniform coherent readonly highp image2D g_image; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + mg_FragColor = imageLoad(g_image, ivec2(0)); +} +)"; + const String first = SplitReadWriteImageUniforms(stage); + const String second = SplitReadWriteImageUniforms(stage); + EXPECT_EQ(first, second); + EXPECT_TRUE(Contains(first, "uniform coherent readonly highp image2D " + RoAlias("g_image") + ";")) + << first; +} + // 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