diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index b262539b..16e2dbc8 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -882,6 +882,27 @@ namespace MobileGL::MG_Backend::DirectGLES { SizeT length; String text; }; + + // The offset just past the `;` that terminates the call whose argument list opens at + // `openParen`, or npos when what follows is not a plain statement. Parentheses alone + // are counted: every other bracket a GLSL argument list can contain is balanced + // inside them, and imageStore returns void, so a well-formed call site is always + // `imageStore(...);` and anything else is a shape this pass declines to edit. + SizeT FindEndOfCallStatement(const String& code, SizeT openParen) { + Int depth = 0; + SizeT scan = openParen; + for (; scan < code.size(); ++scan) { + if (code[scan] == '(') { + ++depth; + } else if (code[scan] == ')' && --depth == 0) { + break; + } + } + if (scan >= code.size()) return String::npos; + const SizeT after = code.find_first_not_of(" \t\r\n", scan + 1); + if (after == String::npos || code[after] != ';') return String::npos; + return after + 1; + } } // namespace String SplitReadWriteImageUniforms(const String& glslCode) { @@ -949,6 +970,7 @@ namespace MobileGL::MG_Backend::DirectGLES { SizeT declIndex; SizeT start; SizeT length; + SizeT callOpen; // the '(' of the call this argument belongs to }; Vector storeSites; for (SizeT pos = glslCode.find("image"); pos != String::npos; pos = glslCode.find("image", pos + 1)) { @@ -998,7 +1020,7 @@ namespace MobileGL::MG_Backend::DirectGLES { break; case ImageBuiltinAccess::Store: decl.stored = true; - storeSites.push_back({declIndex, argStart, argEnd - argStart}); + storeSites.push_back({declIndex, argStart, argEnd - argStart, openParen}); break; case ImageBuiltinAccess::None: break; @@ -1047,6 +1069,24 @@ namespace MobileGL::MG_Backend::DirectGLES { const ImageUniformDecl& decl = decls[site.declIndex]; if (!decl.split) continue; edits.push_back({site.start, site.length, decl.writeName}); + // ...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 + // is that the shader both stores and loads through it, which on the ES side is now + // a write to one variable followed by a read of another the compiler has no reason + // to believe alias. Adreno duly serves the load from before the store + // (KHR-GL4x.shader_image_load_store.advanced-memory-order's store/load/compare + // loop reads back the previous iteration's value). memoryBarrierImage() is the + // GLSL primitive for exactly that ordering, is core GLSL ES 3.10 in every stage, + // and is not an execution barrier, so it is legal in non-uniform control flow too. + // + // Confined to the split pair: a single-declaration repair has nothing aliasing it + // and must not pay for this, and a shader that never got split never sees it at + // all. + const SizeT statementEnd = FindEndOfCallStatement(glslCode, site.callOpen); + if (statementEnd != String::npos) { + edits.push_back({statementEnd, 0, " memoryBarrierImage();"}); + } } if (edits.empty()) { return glslCode; diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 063651d2..9520d243 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -197,8 +197,9 @@ namespace MobileGL::MG_Backend::DirectGLES { // * stored only -> add `writeonly` // * both -> emit TWO declarations on the same binding and of the // same type, `coherent readonly ` and `coherent - // writeonly `, and point - // every imageStore at the second one. Several image + // writeonly `, point + // every imageStore at the second one, and follow each of + // those stores with `memoryBarrierImage();`. 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. @@ -209,6 +210,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // read-after-write cross-variable. The single-declaration repairs above do not get it - // nothing aliases them. // + // The barrier is the other half of the same problem, and coherent alone did not cover it: + // visibility is not ORDER. Within one invocation the ES compiler sees a write to one + // variable and a read of another it has no reason to believe alias, and is free to serve + // the read from before the write - which is what advanced-memory-order's store/load/ + // compare loop measured on Adreno. memoryBarrierImage() orders exactly those two, is core + // GLSL ES 3.10 in every stage, and is not an execution barrier, so it is legal in + // non-uniform control flow. It costs something in a shader that stores to a read+write + // image in a loop, which is why it is confined to the split pair. + // // 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 // shader that used to compile into a link failure. ES only guarantees 4 fragment image diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index 15d4b524..c8e6702b 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -233,6 +233,66 @@ void main() EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D storeOnly;")) << out; } +// The ORDERING half of the split, which `coherent` alone does not buy. Coherent makes the store +// through one variable VISIBLE to a load through the other; it says nothing about the order of +// the two within a single invocation, and the ES compiler - seeing a write to one variable and a +// read of another it has no reason to believe alias - is free to serve the read from before the +// write. That is what advanced-memory-order measured on Adreno with the coherent pair already in +// place. memoryBarrierImage() is the primitive that orders them. +TEST(SplitReadWriteImageUniformsTest, EverySplitStoreIsFollowedByAnImageMemoryBarrier) { + const String source = R"(#version 320 es +layout(binding = 2, rgba8) uniform highp image2D goku; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + imageStore(goku, ivec2(0), vec4(1.0)); + highp vec4 first = imageLoad(goku, ivec2(0)); + imageStore(goku, ivec2(0), vec4(2.0)); + mg_FragColor = first + imageLoad(goku, ivec2(0)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ", ivec2(0), vec4(1.0)); memoryBarrierImage();")) + << out; + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("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; +} + +// The barrier belongs to the SPLIT alone. A store-only image was repaired in place, nothing +// aliases it, and paying for a barrier there would slow down every shader that merely writes an +// image - which is most of them. +TEST(SplitReadWriteImageUniformsTest, ARepairedButUnsplitStoreGetsNoBarrier) { + const String source = R"(#version 320 es +layout(binding = 3, rgba8) uniform highp image2D storeOnly; +void main() +{ + imageStore(storeOnly, ivec2(0), vec4(1.0)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "uniform writeonly highp image2D storeOnly;")) << out; + EXPECT_FALSE(Contains(out, "memoryBarrierImage")) << out; +} + +// The store site is found by matching the call's own parentheses, not by looking for the next +// ')', so a nested call in the value argument does not truncate the statement and the barrier +// still lands after the whole thing. +TEST(SplitReadWriteImageUniformsTest, TheBarrierLandsAfterAStoreWithNestedParentheses) { + const String source = R"(#version 320 es +layout(binding = 6, rgba8) uniform highp image2D gohan[3]; +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; + EXPECT_EQ(CountOf(out, "memoryBarrierImage();"), 1u) << out; +} + // imageSize reads no texels and writes none, so it decides nothing; readonly is what keeps // such a declaration legal. TEST(SplitReadWriteImageUniformsTest, ImageSizeAloneDoesNotCountAsALoadOrAStore) {