swung0x48/MobileGL
1
0
Fork 0
mirror of https://github.com/MobileGL-Dev/MobileGL synced 2026-09-07 19:58:32 +09:00
Code Issues Packages Projects Releases Wiki Activity

[Fix, Test] (DirectGLES): order the split image pair's store before its load with memoryBarrierImage

Browse Source
This commit is contained in:
swung0x48
2026-08-20 22:49:05 -04:00
parent 4154f2e941
commit 8ae93c837d
3 changed files with 113 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Backend/DirectGLES/Utils.cpp
+41 -1
View File
@@ -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<StoreSite> 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;
MobileGL/MG_Backend/DirectGLES/Utils.h
+12 -2
View File
@@ -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 <name>` and `coherent
// writeonly <IMAGE_WRITE_ALIAS_PREFIX><name>`, and point
// every imageStore at the second one. Several image
// writeonly <IMAGE_WRITE_ALIAS_PREFIX><name>`, 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
MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp
+60
View File
@@ -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) {