[Fix, Test] (DirectGLES): make both halves of a split read+write image coherent

MobileGL/MG_Backend/DirectGLES/Utils.cpp

@@ -822,9 +822,19 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // A rebuilt declaration. Keeps SPIRV-Cross's own word order (`uniform readonly
             // highp image2D`) so the image-rebinding regex in Managers.cpp still matches what
             // comes out of here, whichever order the two passes end up running in.
+            //
+            // `forceCoherent` is for the SPLIT pair only. GLSL guarantees that a write through
+            // one image variable is visible to a read through a DIFFERENT one only when both are
+            // declared coherent, and the split turns a same-variable read-after-write - which
+            // desktop GLSL orders by construction, so the source almost never says `coherent` -
+            // into exactly that cross-variable shape. Without it the driver may serve the load
+            // from a cache that never saw the store through the writeonly half.
             String BuildImageDeclaration(const ImageUniformDecl& decl, const char* memoryQualifier,
-                                         const String& variableName) {
+                                         const String& variableName, Bool forceCoherent = false) {
                 String out = "layout(" + decl.layout + ") uniform ";
+                if (forceCoherent && !ContainsIdentifier(decl.qualifiers, "coherent")) {
+                    out += "coherent ";
+                }
                 out += memoryQualifier;
                 out += ' ';
                 if (!decl.qualifiers.empty()) {
@@ -1008,9 +1018,15 @@ namespace MobileGL::MG_Backend::DirectGLES {
                     decl.writeName = MakeImageWriteAliasName(decl.name, glslCode, takenAliases);
                     takenAliases.push_back(decl.writeName);
                     decl.split = true;
+                    // Both halves carry `coherent`; see BuildImageDeclaration. The
+                    // single-declaration cases below stay as they were - nothing aliases them, so
+                    // 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) + "\n" +
+                                     BuildImageDeclaration(decl, "readonly", decl.name, /*forceCoherent=*/true) +
-                                         BuildImageDeclaration(decl, "writeonly", decl.writeName)});
+                                         "\n" +
+                                         BuildImageDeclaration(decl, "writeonly", decl.writeName,
+                                                               /*forceCoherent=*/true)});
                 } else if (decl.stored) {
                     edits.push_back({decl.declStart, decl.declLength,
                                      BuildImageDeclaration(decl, "writeonly", decl.name)});

MobileGL/MG_Backend/DirectGLES/Utils.h

@@ -196,11 +196,18 @@ namespace MobileGL::MG_Backend::DirectGLES {
         //  * loaded only            -> add `readonly`
         //  * stored only            -> add `writeonly`
         //  * both                   -> emit TWO declarations on the same binding and of the
-        //                              same type, `readonly <name>` and `writeonly
+        //                              same type, `coherent readonly <name>` and `coherent
-        //                              <IMAGE_WRITE_ALIAS_PREFIX><name>`, and point every
+        //                              writeonly <IMAGE_WRITE_ALIAS_PREFIX><name>`, and point
-        //                              imageStore at the second one. Several image variables
+        //                              every imageStore at the second one. Several image
-        //                              may share an image unit as long as they have the same
+        //                              variables may share an image unit as long as they have
-        //                              type and format, which is exactly what the pair is.
+        //                              the same type and format, which is exactly what the pair
+        //                              is.
+        //
+        // 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
+        // read-after-write cross-variable. The single-declaration repairs above do not get it -
+        // nothing aliases them.
         //
         // Budget note: the split DOUBLES the image-uniform count of the stage it fires in, so
         // a driver advertising a tight GL_MAX_{FRAGMENT,VERTEX,...}_IMAGE_UNIFORMS can turn a

MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp

@@ -59,9 +59,11 @@ void main()
     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.
+    // variables on one image unit legal - and both `coherent`, which is what makes the store
-    EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform readonly highp image2D goku;"));
+    // through one of them visible to the load through the other.
-    EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform writeonly highp image2D " + WriteAlias("goku") + ";"));
+    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") + ";"));
 
     // The load keeps the original name, the store moves to the writeonly half.
     EXPECT_TRUE(Contains(out, "imageLoad(goku,"));
@@ -152,9 +154,9 @@ void main()
 }
 )";
     const String out = SplitReadWriteImageUniforms(source);
-    EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform readonly highp image2D gohan[3];"));
+    EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform coherent readonly highp image2D gohan[3];"));
-    EXPECT_TRUE(Contains(out,
+    EXPECT_TRUE(Contains(
-                         "layout(binding = 6, rgba8) uniform writeonly highp image2D " + WriteAlias("gohan") + "[3];"));
+        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],"));
 }
@@ -174,9 +176,11 @@ void main()
 )";
     const String out = SplitReadWriteImageUniforms(source);
 
-    // goku is read+write -> split; goku_hd is write-only -> qualified in place, not split.
+    // goku is read+write -> split (and coherent with it); goku_hd is write-only -> qualified in
-    EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform readonly highp image2D goku;"));
+    // place, not split, and left non-coherent because nothing aliases it.
-    EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform writeonly highp image2D " + WriteAlias("goku") + ";"));
+    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"));
@@ -197,6 +201,36 @@ void main()
     EXPECT_TRUE(Contains(out, "uniform readonly coherent restrict highp image2D goku;"));
     EXPECT_TRUE(
         Contains(out, "uniform writeonly coherent restrict highp image2D " + WriteAlias("goku") + ";"));
+    // ...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);
+}
+
+// The visibility half of the split, and the reason it is not cosmetic: GLSL orders a
+// same-variable read-after-write within one invocation by construction, but once the store goes
+// through `mg_imageWrite_goku` and the load through `goku` the two are DIFFERENT variables, and
+// the ordering only holds if both are coherent. Desktop sources almost never say so - they had
+// no reason to - which is how KHR-GL4x.shader_image_load_store.advanced-memory-order's
+// store/load/compare loop started reading back the value it had not stored yet.
+TEST(SplitReadWriteImageUniformsTest, SplitPairIsMadeCoherentEvenWhenTheSourceIsNot) {
+    const String source = R"(#version 320 es
+layout(binding = 2, rgba8) uniform highp image2D goku;
+layout(binding = 3, rgba8) uniform highp image2D storeOnly;
+layout(location = 0) out highp vec4 mg_FragColor;
+void main()
+{
+    imageStore(goku, ivec2(0), vec4(1.0));
+    mg_FragColor = imageLoad(goku, ivec2(0));
+    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;
+    // 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;
 }
 
 // imageSize reads no texels and writes none, so it decides nothing; readonly is what keeps