From 421ccd08c624466bd00999a3d00fef27ddf58dba Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 12:04:43 -0400 Subject: [PATCH] [Fix, Test] (DirectGLES): make both halves of a split read+write image coherent --- MobileGL/MG_Backend/DirectGLES/Utils.cpp | 22 ++++++-- MobileGL/MG_Backend/DirectGLES/Utils.h | 17 ++++-- .../Backend/DirectGLES/EsslShaderPassTest.cpp | 52 +++++++++++++++---- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index c74014ee..3abb7640 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/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, "writeonly", decl.writeName)}); + BuildImageDeclaration(decl, "readonly", decl.name, /*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)}); diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 33f04bd5..063651d2 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/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 ` and `writeonly - // `, and point every - // imageStore at the second one. 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. + // same type, `coherent readonly ` and `coherent + // writeonly `, and point + // every imageStore at the second one. 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 `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 diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 6ce2facd..15d4b524 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/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. - EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform readonly highp image2D goku;")); - EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform writeonly highp image2D " + WriteAlias("goku") + ";")); + // 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") + ";")); // 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 writeonly highp image2D " + WriteAlias("gohan") + "[3];")); + 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],")); } @@ -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. - EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform readonly highp image2D goku;")); - EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform writeonly highp image2D " + WriteAlias("goku") + ";")); + // 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")); @@ -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