[Merge] (DirectGLES): take the image-alias rename under the format carriers

MobileGL/MG_Backend/DirectGLES/Utils.cpp

@@ -996,11 +996,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
@@ -1337,10 +1343,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.
+                const Bool hasReadonly = ContainsIdentifier(qualifiers, "readonly");
-                if (ContainsIdentifier(qualifiers, "readonly") || ContainsIdentifier(qualifiers, "writeonly")) {
+                const Bool hasWriteonly = ContainsIdentifier(qualifiers, "writeonly");
-                    continue;
+                // 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;
@@ -1349,10 +1357,12 @@ namespace MobileGL::MG_Backend::DirectGLES {
                     hasFormat = true;
                     exemptFormat = IsMemoryQualifierExemptImageFormat(token);
                 }
-                // No format qualifier at all is a different (and, in ES, unconditionally
+                // A declaration carrying neither qualifier is illegal ES unless its format is
-                // illegal) shape that GL_EXT_shader_image_load_formatted would be needed for;
+                // r32f/r32i/r32ui, and no format qualifier at all is a shape SPIRV-Cross refuses
-                // SPIRV-Cross refuses to emit it for an ES target, so nothing to do here.
+                // to emit for an ES target. Either way there is no repair to make - and no rename
-                if (!hasFormat || exemptFormat) continue;
+                // 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();
@@ -1362,6 +1372,10 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 decl.arraySuffix = NormalizeDeclarationSpacing(match[5].str());
                 decl.declStart = static_cast<SizeT>(match.position(0));
                 decl.declLength = match[0].str().size();
+                decl.nameStart = static_cast<SizeT>(match.position(4));
+                decl.nameLength = match[4].str().size();
+                decl.preTaggedReadonly = hasReadonly;
+                decl.preTaggedWriteonly = hasWriteonly;
                 decls.push_back(Move(decl));
             }
             if (decls.empty()) {
@@ -1493,6 +1507,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;

MobileGL/MG_Backend/DirectGLES/Utils.h

@@ -444,8 +444,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
+        // A declaration SPIRV-Cross already tagged `readonly` or `writeonly` needs no qualifier
-        // ones that already agree across stages.
+        // 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

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.
+// A declaration SPIRV-Cross already qualified needs no REPAIR - but it still needs the rename.
-TEST(SplitReadWriteImageUniformsTest, AlreadyQualifiedDeclarationsAreUntouched) {
+// 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
+    const String out = SplitReadWriteImageUniforms(source);
-    // source carries the SAME one in every stage, so there is no cross-stage mismatch to break up
+    EXPECT_TRUE(Contains(out, "layout(binding = 0, rgba8) uniform readonly highp image2D " +
-    // and renaming it would only churn the text.
+                                  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