From a51c68bb2c052f0bd0d5a88003bea82cfffb9222 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 17:50:25 -0400 Subject: [PATCH 1/3] [Fix, Test] (MG_Util, MG_IntegrationTest): a 64-bit float constant was read as 32 bits, so every comparison against a round double became an epsilon test against zero --- .../Scenarios/DoublePrecisionScenario.cpp | 324 ++++++++++++++++++ .../ShaderTranspiler/DemoteFloat64Test.cpp | 50 +++ .../EliminateFloatEqualsZeroPass.cpp | 32 +- 3 files changed, 403 insertions(+), 3 deletions(-) diff --git a/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp index cb782e31..a59e3d69 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp @@ -99,6 +99,8 @@ void main() { void TearDown() override { if (!Ready()) return; + if (m_shapeOutput != 0) glDeleteBuffers(1, &m_shapeOutput); + if (m_shapeProgram != 0) glDeleteProgram(m_shapeProgram); if (m_output != 0) glDeleteBuffers(1, &m_output); if (m_program != 0) glDeleteProgram(m_program); } @@ -146,9 +148,147 @@ void main() { unsigned int m_program = 0; unsigned int m_output = 0; + unsigned int m_shapeProgram = 0; + unsigned int m_shapeOutput = 0; std::string m_buildLog; }; + // Every double-typed uniform shape GLSL has, all thirteen of them, in one program - the + // shape of KHR-GL43.compute_shader.fp64-case2. The scalar and the square matrices are + // covered by the cases above; what only a set like this reaches is the NON-SQUARE + // matrices, whose column stride and total size both change when the demotion turns a + // 64-bit column into a 32-bit one, and whose members therefore move every uniform + // declared after them. + // + // The shader reports every component separately rather than one pass/fail flag, because + // "the readback is wrong" is not a diagnosis: a wrong column stride, a wrong member + // offset and a wrong narrowing all fail the same single comparison, and only the + // component map says which. + // No #version here on purpose: it is handed over as a separate source string, the way + // the CTS case hands it over. + constexpr const char* kAllDoubleShapesSource = R"( +layout(local_size_x = 1) in; +uniform double g_0; +uniform dvec2 g_1; +uniform dvec3 g_2; +uniform dvec4 g_3; +uniform dmat2 g_4; +uniform dmat2x3 g_5; +uniform dmat2x4 g_6; +uniform dmat3x2 g_7; +uniform dmat3 g_8; +uniform dmat3x4 g_9; +uniform dmat4x2 g_10; +uniform dmat4x3 g_11; +uniform dmat4 g_12; +layout(std430, binding = 0) buffer Output { + float g_out[]; +}; +void main() { + g_out[0] = float(g_0); + for (int i = 0; i < 2; ++i) g_out[1 + i] = float(g_1[i]); + for (int i = 0; i < 3; ++i) g_out[3 + i] = float(g_2[i]); + for (int i = 0; i < 4; ++i) g_out[6 + i] = float(g_3[i]); + for (int c = 0; c < 2; ++c) for (int r = 0; r < 2; ++r) g_out[10 + c * 2 + r] = float(g_4[c][r]); + for (int c = 0; c < 2; ++c) for (int r = 0; r < 3; ++r) g_out[14 + c * 3 + r] = float(g_5[c][r]); + for (int c = 0; c < 2; ++c) for (int r = 0; r < 4; ++r) g_out[20 + c * 4 + r] = float(g_6[c][r]); + for (int c = 0; c < 3; ++c) for (int r = 0; r < 2; ++r) g_out[28 + c * 2 + r] = float(g_7[c][r]); + for (int c = 0; c < 3; ++c) for (int r = 0; r < 3; ++r) g_out[34 + c * 3 + r] = float(g_8[c][r]); + for (int c = 0; c < 3; ++c) for (int r = 0; r < 4; ++r) g_out[43 + c * 4 + r] = float(g_9[c][r]); + for (int c = 0; c < 4; ++c) for (int r = 0; r < 2; ++r) g_out[55 + c * 2 + r] = float(g_10[c][r]); + for (int c = 0; c < 4; ++c) for (int r = 0; r < 3; ++r) g_out[63 + c * 3 + r] = float(g_11[c][r]); + for (int c = 0; c < 4; ++c) for (int r = 0; r < 4; ++r) g_out[75 + c * 4 + r] = float(g_12[c][r]); +} +)"; + + // The values the CTS case sets, spelled the way it spells them - column-major, and small + // enough that every one is exact in a float. Nothing here is a precision question; a + // component that comes back wrong came back from the wrong bytes. + constexpr double kG0 = 1.0; + constexpr double kG1[2] = {2.0, 3.0}; + constexpr double kG2[3] = {4.0, 5.0, 6.0}; + constexpr double kG3[4] = {7.0, 8.0, 9.0, 10.0}; + constexpr double kG4[4] = {11.0, 12.0, 13.0, 14.0}; + constexpr double kG5[6] = {15.0, 16.0, 17.0, 18.0, 19.0, 20.0}; + constexpr double kG6[8] = {21.0, 22.0, 23.0, 24.0, 25.0, 26.0, 27.0, 28.0}; + constexpr double kG7[6] = {29.0, 30.0, 31.0, 32.0, 33.0, 34.0}; + constexpr double kG8[9] = {35.0, 36.0, 37.0, 38.0, 39.0, 40.0, 41.0, 42.0, 43.0}; + constexpr double kG9[12] = {44.0, 45.0, 46.0, 47.0, 48.0, 49.0, 50.0, 51.0, 52.0, 53.0, 54.0, 55.0}; + constexpr double kG10[8] = {56.0, 57.0, 58.0, 59.0, 60.0, 61.0, 62.0, 63.0}; + constexpr double kG11[12] = {63.0, 64.0, 65.0, 66.0, 67.0, 68.0, 69.0, 70.0, 71.0, 27.0, 73.0, 74.0}; + constexpr double kG12[16] = {75.0, 76.0, 77.0, 78.0, 79.0, 80.0, 81.0, 82.0, + 83.0, 84.0, 85.0, 86.0, 87.0, 88.0, 89.0, 90.0}; + + struct DoubleShape { + const char* name; + int base; + int columns; // 1 for the scalar and the vectors + int rows; // component count for the scalar and the vectors + const double* values; + }; + + constexpr DoubleShape kDoubleShapes[] = { + {"g_0 double", 0, 1, 1, &kG0}, {"g_1 dvec2", 1, 1, 2, kG1}, + {"g_2 dvec3", 3, 1, 3, kG2}, {"g_3 dvec4", 6, 1, 4, kG3}, + {"g_4 dmat2", 10, 2, 2, kG4}, {"g_5 dmat2x3", 14, 2, 3, kG5}, + {"g_6 dmat2x4", 20, 2, 4, kG6}, {"g_7 dmat3x2", 28, 3, 2, kG7}, + {"g_8 dmat3", 34, 3, 3, kG8}, {"g_9 dmat3x4", 43, 3, 4, kG9}, + {"g_10 dmat4x2", 55, 4, 2, kG10}, {"g_11 dmat4x3", 63, 4, 3, kG11}, + {"g_12 dmat4", 75, 4, 4, kG12}, + }; + + constexpr int kAllShapeSlots = 91; + + // The conformance case's own shader, kept verbatim down to the literal suffixes and the + // unnamed, unqualified storage block - except that each comparison sets its OWN bit + // instead of collapsing all thirteen into one flag. That single flag is the whole reason + // the case was unexplained for a wave: it says "something is wrong" and nothing else. + // + // Verbatim matters here. Reading the components out one at a time (the case above) + // passes; whatever fails does so through the shape the conformance case actually + // writes - whole-matrix comparison against a constructor, a storage block with no + // layout qualifier and no instance name, values reached with constant indices. + constexpr const char* kCtsShapedSource = R"( +layout(local_size_x = 1) in; +buffer Result { + int g_result; +}; +uniform double g_0; +uniform dvec2 g_1; +uniform dvec3 g_2; +uniform dvec4 g_3; +uniform dmat2 g_4; +uniform dmat2x3 g_5; +uniform dmat2x4 g_6; +uniform dmat3x2 g_7; +uniform dmat3 g_8; +uniform dmat3x4 g_9; +uniform dmat4x2 g_10; +uniform dmat4x3 g_11; +uniform dmat4 g_12; + +void main() { + g_result = 0; + + if (g_0 != 1.0LF) g_result |= 1; + if (g_1 != dvec2(2.0LF, 3.0LF)) g_result |= 2; + if (g_2 != dvec3(4.0LF, 5.0LF, 6.0LF)) g_result |= 4; + if (g_3 != dvec4(7.0LF, 8.0LF, 9.0LF, 10.0LF)) g_result |= 8; + + if (g_4 != dmat2(11.0LF, 12.0LF, 13.0LF, 14.0LF)) g_result |= 16; + if (g_5 != dmat2x3(15.0LF, 16.0LF, 17.0LF, 18.0LF, 19.0LF, 20.0LF)) g_result |= 32; + if (g_6 != dmat2x4(21.0LF, 22.0LF, 23.0LF, 24.0LF, 25.0LF, 26.0LF, 27.0LF, 28.0LF)) g_result |= 64; + + if (g_7 != dmat3x2(29.0LF, 30.0LF, 31.0LF, 32.0LF, 33.0LF, 34.0LF)) g_result |= 128; + if (g_8 != dmat3(35.0LF, 36.0LF, 37.0LF, 38.0LF, 39.0LF, 40.0LF, 41.0LF, 42.0LF, 43.0LF)) g_result |= 256; + if (g_9 != dmat3x4(44.0LF, 45.0LF, 46.0LF, 47.0LF, 48.0LF, 49.0LF, 50.0LF, 51.0LF, 52.0LF, 53.0LF, 54.0LF, 55.0LF)) g_result |= 512; + + if (g_10 != dmat4x2(56.0, 57.0, 58.0, 59.0, 60.0, 61.0, 62.0, 63.0)) g_result |= 1024; + if (g_11 != dmat4x3(63.0, 64.0, 65.0, 66.0, 67.0, 68.0, 69.0, 70.0, 71.0, 27.0, 73, 74.0)) g_result |= 2048; + if (g_12 != dmat4(75.0, 76.0, 77.0, 78.0, 79.0, 80.0, 81.0, 82.0, 83.0, 84.0, 85.0, 86.0, 87.0, 88.0, 89.0, 90.0)) g_result |= 4096; +} +)"; + TEST_F(DoublePrecisionScenario, ADoubleUniformReachesTheShaderAtFloatPrecision) { if (!Ready()) return; glUseProgram(m_program); @@ -353,6 +493,190 @@ void main() { EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); } + TEST_F(DoublePrecisionScenario, EveryDoubleUniformShapeArrivesWhereTheShaderReadsIt) { + if (!Ready()) return; + // Built the way the CTS case builds it, because every step of that build has been a + // bug here at least once: the source arrives as TWO strings (the version directive + // and the body), the shader is attached before it has a source and deleted while + // still attached, and the program is linked twice. + m_shapeProgram = glCreateProgram(); + ASSERT_NE(m_shapeProgram, 0u); + { + const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); + glAttachShader(m_shapeProgram, shader); + glDeleteShader(shader); + const char* const sources[2] = {"#version 430 core\n", kAllDoubleShapesSource}; + glShaderSource(shader, 2, sources, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled == GL_FALSE) { + char log[2048] = {}; + glGetShaderInfoLog(shader, sizeof(log) - 1, nullptr, log); + FAIL() << "compute shader did not compile: " << log; + } + } + glLinkProgram(m_shapeProgram); + { + GLint linkedOnce = 0; + glGetProgramiv(m_shapeProgram, GL_LINK_STATUS, &linkedOnce); + if (linkedOnce == GL_FALSE) { + char log[2048] = {}; + glGetProgramInfoLog(m_shapeProgram, sizeof(log) - 1, nullptr, log); + FAIL() << "compute program did not link: " << log; + } + } + + glGenBuffers(1, &m_shapeOutput); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, m_shapeOutput); + const std::vector zeroes(kAllShapeSlots, 0.0f); + glBufferData(GL_SHADER_STORAGE_BUFFER, kAllShapeSlots * sizeof(float), zeroes.data(), GL_DYNAMIC_DRAW); + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, m_shapeOutput); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, 0); + + const auto location = [&](const char* name) { return glGetUniformLocation(m_shapeProgram, name); }; + + // Pass one sets through glProgramUniform*, pass two through glUniform* after a + // re-link - the two entry-point families the CTS case exercises, and two different + // routes into the same uniform storage. + const auto setWithProgramUniform = [&]() { + glProgramUniform1d(m_shapeProgram, location("g_0"), kG0); + glProgramUniform2d(m_shapeProgram, location("g_1"), kG1[0], kG1[1]); + glProgramUniform3d(m_shapeProgram, location("g_2"), kG2[0], kG2[1], kG2[2]); + glProgramUniform4d(m_shapeProgram, location("g_3"), kG3[0], kG3[1], kG3[2], kG3[3]); + glProgramUniformMatrix2dv(m_shapeProgram, location("g_4"), 1, GL_FALSE, kG4); + glProgramUniformMatrix2x3dv(m_shapeProgram, location("g_5"), 1, GL_FALSE, kG5); + glProgramUniformMatrix2x4dv(m_shapeProgram, location("g_6"), 1, GL_FALSE, kG6); + glProgramUniformMatrix3x2dv(m_shapeProgram, location("g_7"), 1, GL_FALSE, kG7); + glProgramUniformMatrix3dv(m_shapeProgram, location("g_8"), 1, GL_FALSE, kG8); + glProgramUniformMatrix3x4dv(m_shapeProgram, location("g_9"), 1, GL_FALSE, kG9); + glProgramUniformMatrix4x2dv(m_shapeProgram, location("g_10"), 1, GL_FALSE, kG10); + glProgramUniformMatrix4x3dv(m_shapeProgram, location("g_11"), 1, GL_FALSE, kG11); + glProgramUniformMatrix4dv(m_shapeProgram, location("g_12"), 1, GL_FALSE, kG12); + }; + // Deliberately does NOT re-issue glUseProgram: the CTS case leaves the program + // current across the re-link and writes into it from there, so this is the path + // where a re-link has to keep the current program's uniform storage addressable. + const auto setWithUniform = [&]() { + glUniform1d(location("g_0"), kG0); + glUniform2d(location("g_1"), kG1[0], kG1[1]); + glUniform3d(location("g_2"), kG2[0], kG2[1], kG2[2]); + glUniform4d(location("g_3"), kG3[0], kG3[1], kG3[2], kG3[3]); + glUniformMatrix2dv(location("g_4"), 1, GL_FALSE, kG4); + glUniformMatrix2x3dv(location("g_5"), 1, GL_FALSE, kG5); + glUniformMatrix2x4dv(location("g_6"), 1, GL_FALSE, kG6); + glUniformMatrix3x2dv(location("g_7"), 1, GL_FALSE, kG7); + glUniformMatrix3dv(location("g_8"), 1, GL_FALSE, kG8); + glUniformMatrix3x4dv(location("g_9"), 1, GL_FALSE, kG9); + glUniformMatrix4x2dv(location("g_10"), 1, GL_FALSE, kG10); + glUniformMatrix4x3dv(location("g_11"), 1, GL_FALSE, kG11); + glUniformMatrix4dv(location("g_12"), 1, GL_FALSE, kG12); + }; + + const auto dispatchAndRead = [&]() { + glUseProgram(m_shapeProgram); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_BUFFER_UPDATE_BARRIER_BIT); + std::vector values(kAllShapeSlots, -1.0f); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, m_shapeOutput); + glGetBufferSubData(GL_SHADER_STORAGE_BUFFER, 0, kAllShapeSlots * sizeof(float), values.data()); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, 0); + // The program stays current on purpose - see setWithUniform. + return values; + }; + + const auto expectEverything = [](const std::vector& values, const char* pass) { + for (const DoubleShape& shape : kDoubleShapes) { + for (int c = 0; c < shape.columns; ++c) { + for (int r = 0; r < shape.rows; ++r) { + const int component = c * shape.rows + r; + EXPECT_FLOAT_EQ(values[shape.base + component], + static_cast(shape.values[component])) + << pass << ": " << shape.name << " column " << c << " row " << r; + } + } + } + }; + + setWithProgramUniform(); + expectEverything(dispatchAndRead(), "glProgramUniform*"); + + // A re-link zeroes every uniform, so pass two proves its own writes rather than + // reading pass one's bytes back. + glLinkProgram(m_shapeProgram); + GLint linked = 0; + glGetProgramiv(m_shapeProgram, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, m_shapeOutput); + glBufferSubData(GL_SHADER_STORAGE_BUFFER, 0, kAllShapeSlots * sizeof(float), zeroes.data()); + glBindBuffer(GL_SHADER_STORAGE_BUFFER, 0); + + setWithUniform(); + expectEverything(dispatchAndRead(), "glUniform* after re-link"); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + } + + TEST_F(DoublePrecisionScenario, TheConformanceUniformShaderAgreesWithEveryValueItWasGiven) { + if (!Ready()) return; + m_shapeProgram = glCreateProgram(); + ASSERT_NE(m_shapeProgram, 0u); + { + const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); + glAttachShader(m_shapeProgram, shader); + glDeleteShader(shader); + const char* const sources[2] = {"#version 430 core\n", kCtsShapedSource}; + glShaderSource(shader, 2, sources, nullptr); + glCompileShader(shader); + GLint compiled = 0; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled == GL_FALSE) { + char log[2048] = {}; + glGetShaderInfoLog(shader, sizeof(log) - 1, nullptr, log); + FAIL() << "compute shader did not compile: " << log; + } + } + glLinkProgram(m_shapeProgram); + GLint linked = 0; + glGetProgramiv(m_shapeProgram, GL_LINK_STATUS, &linked); + if (linked == GL_FALSE) { + char log[2048] = {}; + glGetProgramInfoLog(m_shapeProgram, sizeof(log) - 1, nullptr, log); + FAIL() << "compute program did not link: " << log; + } + + glGenBuffers(1, &m_shapeOutput); + const GLint seed = 123; + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, m_shapeOutput); + glBufferData(GL_SHADER_STORAGE_BUFFER, sizeof(seed), &seed, GL_STATIC_DRAW); + + const auto location = [&](const char* name) { return glGetUniformLocation(m_shapeProgram, name); }; + glProgramUniform1d(m_shapeProgram, location("g_0"), kG0); + glProgramUniform2d(m_shapeProgram, location("g_1"), kG1[0], kG1[1]); + glProgramUniform3d(m_shapeProgram, location("g_2"), kG2[0], kG2[1], kG2[2]); + glProgramUniform4d(m_shapeProgram, location("g_3"), kG3[0], kG3[1], kG3[2], kG3[3]); + glProgramUniformMatrix2dv(m_shapeProgram, location("g_4"), 1, GL_FALSE, kG4); + glProgramUniformMatrix2x3dv(m_shapeProgram, location("g_5"), 1, GL_FALSE, kG5); + glProgramUniformMatrix2x4dv(m_shapeProgram, location("g_6"), 1, GL_FALSE, kG6); + glProgramUniformMatrix3x2dv(m_shapeProgram, location("g_7"), 1, GL_FALSE, kG7); + glProgramUniformMatrix3dv(m_shapeProgram, location("g_8"), 1, GL_FALSE, kG8); + glProgramUniformMatrix3x4dv(m_shapeProgram, location("g_9"), 1, GL_FALSE, kG9); + glProgramUniformMatrix4x2dv(m_shapeProgram, location("g_10"), 1, GL_FALSE, kG10); + glProgramUniformMatrix4x3dv(m_shapeProgram, location("g_11"), 1, GL_FALSE, kG11); + glProgramUniformMatrix4dv(m_shapeProgram, location("g_12"), 1, GL_FALSE, kG12); + + glUseProgram(m_shapeProgram); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_BUFFER_UPDATE_BARRIER_BIT); + + GLint disagreements = -1; + glGetBufferSubData(GL_SHADER_STORAGE_BUFFER, 0, sizeof(disagreements), &disagreements); + for (int bit = 0; bit < 13; ++bit) { + EXPECT_EQ(disagreements & (1 << bit), 0) + << kDoubleShapes[bit].name << " did not compare equal to the value it was given"; + } + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + } + TEST_F(DoublePrecisionScenario, TheFp64ExtensionIsNotAdvertised) { if (!Ready()) return; // The shader above compiled, linked and ran without the extension string, which is diff --git a/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp b/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp index 67dc8256..b1dff973 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp @@ -484,3 +484,53 @@ TEST_F(DemoteFloat64Test, RejectsGarbageInput) { Vector output; EXPECT_FALSE(ShaderCompiler::DemoteFloat64ToFloat32(notSpirv, output)); } + +// EliminateFloatEqualsZeroPass turns a comparison against 0.0 into an epsilon test, a +// workaround for drivers whose exact float compare misbehaves. Deciding WHICH constants are +// zero used to read every float constant as though it were 32 bits wide, and on a 64-bit +// constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf and +// every other round double a shader is likely to spell. Each of those was mistaken for 0.0, so +// a comparison against 1.0lf became an epsilon test against ZERO, and came out true for a +// uniform holding exactly 1.0. That is the whole of KHR-GL43.compute_shader.fp64-case2. +// +// Asserted on the optimized module rather than through a driver, because that is where the +// rewrite happens and its fingerprint there is unambiguous: the epsilon form introduces a +// GLSL.std.450 FAbs, and nothing else in these shaders would. +namespace { + Bool RewritesToAnEpsilonTest(const String& source) { + const Vector input = CompileToSpirv(GL_COMPUTE_SHADER, source); + EXPECT_FALSE(input.empty()); + if (input.empty()) return false; + Vector output; + EXPECT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(input, output)); + return Disassemble(output).find("FAbs") != String::npos; + } + + String CompareAgainst(const String& type, const String& literal) { + return "#version 430 core\n" + "layout(local_size_x = 1) in;\n" + "buffer Result { int g_result; };\n" + "uniform " + type + " g_0;\n" + "void main() {\n" + " g_result = 0;\n" + " if (g_0 != " + literal + ") g_result = 1;\n" + "}\n"; + } +} // namespace + +TEST_F(DemoteFloat64Test, AComparisonAgainstANonZeroDoubleIsLeftAlone) { + EXPECT_FALSE(RewritesToAnEpsilonTest(CompareAgainst("double", "1.0LF"))) + << "a double compared against 1.0lf was rewritten into an epsilon test against zero"; +} + +TEST_F(DemoteFloat64Test, AComparisonAgainstZeroIsStillRewritten) { + EXPECT_TRUE(RewritesToAnEpsilonTest(CompareAgainst("double", "0.0LF"))) + << "the rewrite must still fire for a genuine comparison against zero"; +} + +TEST_F(DemoteFloat64Test, TheThirtyTwoBitBehaviourIsUnchanged) { + EXPECT_FALSE(RewritesToAnEpsilonTest(CompareAgainst("float", "1.0"))) + << "a float compared against 1.0 must not be rewritten"; + EXPECT_TRUE(RewritesToAnEpsilonTest(CompareAgainst("float", "0.0"))) + << "the 32-bit behaviour this pass shipped with must be preserved exactly"; +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp index 1405aec2..f16de06a 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp @@ -16,6 +16,7 @@ #include "source/opt/ir_context.h" #include "source/opt/module.h" #include "source/opt/type_manager.h" +#include #include namespace MobileGL { @@ -70,12 +71,37 @@ namespace MobileGL { uint32_t var_id = 0; + // The constant's WIDTH decides which accessor may read it, and asking + // the wrong one does not fail - it answers. + // + // GetFloat() bit-casts words()[0], which only means anything at 32 + // bits. On a 64-bit constant words()[0] is the LOW half of the + // mantissa, and that half is zero for every round double a shader + // actually spells: 1.0lf, 2.0lf, 0.5lf, 100.0lf. Each of those + // therefore looked like 0.0 here, and `d != 1.0lf` was rewritten into + // `abs(d) >= epsilon` - which is TRUE for d == 1.0. That is the whole + // of KHR-GL43.compute_shader.fp64-case2: twelve uniforms compared + // against vector and matrix constructors were untouched (a composite + // is not a FloatConstant) and the one scalar comparison in the shader + // came out inverted. GetFloat() asserts the width, but every shipping + // build compiles with NDEBUG, so the assert never ran. + // + // Widths other than 32 and 64 are declined rather than guessed at: + // GetDoubleValue() reads words()[1], which a 16-bit constant does not + // have. auto is_float_zero = [&](uint32_t id) -> bool { const analysis::Constant* c = const_mgr->FindDeclaredConstant(id); - if (c && c->AsFloatConstant() && fabs(c->AsFloatConstant()->GetFloat()) <= K_EPSILON) { - return true; + if (c == nullptr) return false; + const analysis::FloatConstant* floatConstant = c->AsFloatConstant(); + if (floatConstant == nullptr) return false; + const analysis::Float* floatType = + floatConstant->type() != nullptr ? floatConstant->type()->AsFloat() : nullptr; + if (floatType == nullptr) return false; + switch (floatType->width()) { + case 32: return std::fabs(floatConstant->GetFloatValue()) <= K_EPSILON; + case 64: return std::fabs(floatConstant->GetDoubleValue()) <= K_EPSILON; + default: return false; } - return false; }; if (is_float_zero(op2_id)) { From 49cb1be0fd5f8c1468c51f5607931d3e591929cf Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 17:50:31 -0400 Subject: [PATCH 2/3] [Fix, Test] (MG_Backend/DirectGLES, MG_IntegrationTest): a shader writing a buffer texture through an image unit left the CPU shadow stale, so every map and readback after it saw the old bytes --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 69 ++++++++++ .../Scenarios/BufferTextureScenario.cpp | 130 +++++++++++++++++- 2 files changed, 198 insertions(+), 1 deletion(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index ba4ce5d6..c9b30f2f 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -1326,11 +1327,47 @@ namespace MobileGL::MG_Backend::DirectGLES { backendTarget == TextureTarget::Texture2DMultisampleArray; } + // Which image units currently hold a WRITABLE buffer texture, and how many. Kept here + // rather than recomputed per draw because the frontend tracks 192 image units and + // almost every program uses none of them: the draw path pays one integer test. + // + // Maintained by SyncImageTextureBinding, which is the single funnel for an image-unit + // change on this backend - glBindImageTextures is a frontend loop over + // glBindImageTexture, and the whole-sweep SyncImageTextureBindings goes through it too. + // Anything that clears a binding WITHOUT coming through here (a deleted texture, a + // recreated context) can only leave a bit set for a unit that no longer has one; the + // sweep re-reads the binding, finds nothing to mark, and CLEARS the bit on its way past. + // So the error is self-healing, and in the direction that costs one wasted look rather + // than one missed write. + static Array + g_writableImageBufferUnits{}; + static Uint g_writableImageBufferUnitCount = 0; + + static Bool IsWritableImageBufferTexture(const MG_State::GLState::ImageTextureBinding& binding) { + return binding.Texture != nullptr && binding.Access != GL_READ_ONLY && + binding.Texture->GetStorageType() == TextureStorageType::Buffer; + } + + static void TrackWritableImageBufferUnit(Uint unit, Bool writableBufferTexture) { + Bool& tracked = g_writableImageBufferUnits[unit]; + if (tracked == writableBufferTexture) return; + tracked = writableBufferTexture; + // Written as two guarded steps rather than one signed add: the count is unsigned, + // and a decrement that ever ran one time too many would not saturate at zero, it + // would wrap to four billion and defeat the early-out for the rest of the process. + if (writableBufferTexture) { + ++g_writableImageBufferUnitCount; + } else if (g_writableImageBufferUnitCount > 0) { + --g_writableImageBufferUnitCount; + } + } + void SyncImageTextureBinding(Uint unit) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif auto& imageBinding = MG_State::pGLContext->GetImageTextureBinding(static_cast(unit)); + TrackWritableImageBufferUnit(unit, IsWritableImageBufferTexture(imageBinding)); if (!imageBinding.Texture) { g_GLESFuncs.glBindImageTexture(unit, 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_RGBA8); return; @@ -1343,6 +1380,34 @@ namespace MobileGL::MG_Backend::DirectGLES { layered, imageBinding.Layer, imageBinding.Access, imageBinding.Format); } + // A buffer texture bound to a WRITABLE image unit is a buffer the shader is about to + // write, and those writes land in the ES driver's buffer object - behind the frontend's + // CPU shadow, which is what MapBuffer and GetBufferSubData read. Same flag, and for the + // same reason, as MarkShaderStorageBuffersGpuWritten does for a storage block; the + // difference is only which binding the shader reaches the buffer through. A GL_READ_ONLY + // binding is left alone: marking it would make the next map wait on - and then re-read - + // a dispatch that could not have changed a byte of it. + // + // Called from the draw and dispatch preparations rather than from the eager sync + // glBindImageTexture performs: that one runs before any shader has touched the buffer, + // and flagging there would pull the driver's copy over a shadow the application may + // still be writing into. + void MarkWritableImageBufferTexturesGpuWritten() { + if (g_writableImageBufferUnitCount == 0) return; + for (Uint unit = 0; unit < g_writableImageBufferUnits.size(); ++unit) { + if (!g_writableImageBufferUnits[unit]) continue; + const auto& imageBinding = MG_State::pGLContext->GetImageTextureBinding(static_cast(unit)); + if (!IsWritableImageBufferTexture(imageBinding)) { + TrackWritableImageBufferUnit(unit, false); + continue; + } + auto* textureBuffer = + static_cast(imageBinding.Texture.get()); + const auto& bufferObject = textureBuffer->GetBufferBindingSlot().GetBoundObject(); + if (bufferObject) bufferObject->MarkGpuWritten(); + } + } + void SyncImageTextureBindings() { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); @@ -2258,6 +2323,9 @@ namespace MobileGL::MG_Backend::DirectGLES { syncBit & DrawSyncBit::IndirectBuffer); VertexArrayImpl::SyncCurrentVAO(currentVAO, vaoTwin); TextureImpl::SyncNeccessaryTextures(textureKeys); + // A draw writes through its image units too - the conformance case that found this + // stores into a buffer texture from the FRAGMENT stage, not from a dispatch. + TextureImpl::MarkWritableImageBufferTexturesGpuWritten(); FramebufferImpl::SyncCurrentFBO(); PrgramImpl::SyncCurrentProgram(currentProgram); RenderStateImpl::SyncRenderState(); @@ -3116,6 +3184,7 @@ namespace MobileGL::MG_Backend::DirectGLES { BufferImpl::SyncComputeBuffers(includeDispatchIndirectBuffer); TextureImpl::SyncNeccessaryTextures(textureKeys); TextureImpl::SyncImageTextureBindings(); + TextureImpl::MarkWritableImageBufferTexturesGpuWritten(); PrgramImpl::SyncCurrentProgram(currentProgram); if (!currentProgram || !currentProgram->GetLinkStatus() || !currentProgram->GetSpirvStatus()) { diff --git a/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp index 9079a0c8..83c62bc5 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/BufferTextureScenario.cpp @@ -33,6 +33,7 @@ // branch on. The driver POST's "Buffer textures" row is where that verdict is stated. #include +#include #include #include @@ -73,7 +74,60 @@ out vec4 o_color; void main() { o_color = vec4(float(vFace) / 255.0, 0.0, 0.0, 1.0); } )"; - class BufferTextureScenario : public ScenarioTest {}; + // A buffer texture bound as a WRITABLE image: the shader reads one texel and writes + // another, so a single dispatch proves the read direction (which already worked) and + // the write direction (which is what this exists for) apart from each other. + constexpr const char* kImageBufferCS = R"(#version 430 core +layout(local_size_x = 1) in; +layout(binding = 0, rgba8) uniform imageBuffer uImage; +void main() { + vec4 read = imageLoad(uImage, 1); + imageStore(uImage, 0, vec4(0.0, 1.0, 0.0, 1.0)); + imageStore(uImage, 2, read); +} +)"; + + class BufferTextureScenario : public ScenarioTest { + protected: + bool ComputeImagesAreUsable() const { + GLint maxImageUnits = 0; + GLint maxComputeImageUniforms = 0; + glGetIntegerv(GL_MAX_IMAGE_UNITS, &maxImageUnits); + glGetIntegerv(GL_MAX_COMPUTE_IMAGE_UNIFORMS, &maxComputeImageUniforms); + while (glGetError() != GL_NO_ERROR) { + } + return maxImageUnits >= 1 && maxComputeImageUniforms >= 1; + } + + unsigned int MakeComputeProgram(const char* source) { + const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); + glShaderSource(shader, 1, &source, nullptr); + glCompileShader(shader); + GLint compiled = GL_FALSE; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled == GL_FALSE) { + char log[4096] = {}; + glGetShaderInfoLog(shader, sizeof(log) - 1, nullptr, log); + ADD_FAILURE() << "the compute shader did not compile: " << log; + glDeleteShader(shader); + return 0; + } + const GLuint program = glCreateProgram(); + glAttachShader(program, shader); + glLinkProgram(program); + glDeleteShader(shader); + GLint linked = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + if (linked == GL_FALSE) { + char log[4096] = {}; + glGetProgramInfoLog(program, sizeof(log) - 1, nullptr, log); + ADD_FAILURE() << "the compute program did not link: " << log; + glDeleteProgram(program); + return 0; + } + return program; + } + }; // Draws the full-viewport quad and returns the red byte every fragment was painted with, // or -1 if the quad did not come out uniform (which would mean the flat varying, not the @@ -171,4 +225,78 @@ void main() { o_color = vec4(float(vFace) / 255.0, 0.0, 0.0, 1.0); } EXPECT_EQ(FirstGLError(), 0u); } + // A shader may WRITE a buffer texture too, through an image unit, and the bytes it writes + // land in the backend's buffer - not in the frontend's CPU shadow, which is what MapBuffer + // and GetBufferSubData hand back. A storage-block write is flagged for exactly this reason + // and the shadow is refreshed on the next read; a buffer reached through an image unit is + // the same write through a different binding, and Espryt used to flag only the first, so + // an imageStore into a buffer texture was invisible to every CPU read that followed it - + // silently, with the correct value sitting in the driver's buffer the whole time. + // + // The read direction is asserted in the same dispatch (texel 2 is a copy of texel 1) so a + // failure here cannot be blamed on the image binding not working at all. + TEST_F(BufferTextureScenario, AnImageStoreIntoABufferTextureIsVisibleToTheCpu) { + if (!Ready()) return; + if (!ComputeImagesAreUsable()) GTEST_SKIP() << "no compute image units on this host"; + + constexpr GLuint kRed = 0x000000ffu; // RGBA8 little-endian: r = 255 + constexpr GLuint kGreen = 0xff00ff00u; // what the shader stores: (0, 1, 0, 1) + constexpr int kTexels = 16; + + FirstGLError(); + + const unsigned int program = MakeComputeProgram(kImageBufferCS); + ASSERT_NE(program, 0u); + + const std::vector texels(kTexels, kRed); + GLuint buffer = 0; + glGenBuffers(1, &buffer); + glBindBuffer(GL_TEXTURE_BUFFER, buffer); + glBufferData(GL_TEXTURE_BUFFER, static_cast(texels.size() * sizeof(GLuint)), texels.data(), + GL_DYNAMIC_COPY); + + GLuint texture = 0; + glGenTextures(1, &texture); + glBindTexture(GL_TEXTURE_BUFFER, texture); + glTexBuffer(GL_TEXTURE_BUFFER, GL_RGBA8, buffer); + EXPECT_EQ(FirstGLError(), 0u) << "glTexBuffer(GL_RGBA8) was refused"; + + glBindImageTexture(0, texture, 0, GL_FALSE, 0, GL_READ_WRITE, GL_RGBA8); + EXPECT_EQ(FirstGLError(), 0u) << "glBindImageTexture on a buffer texture was refused"; + + glUseProgram(program); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + + // Both CPU read paths, because they are two entry points onto the same refresh and a + // fix that reaches only one of them is not a fix. Everything below is EXPECT rather than + // ASSERT so that a failure still reaches the cleanup at the end: the harness shares one + // context across every scenario in the process, and a leaked buffer or image binding + // here would surface as a failure somewhere else entirely. + std::vector readBack(kTexels, 0u); + glBindBuffer(GL_TEXTURE_BUFFER, buffer); + glGetBufferSubData(GL_TEXTURE_BUFFER, 0, static_cast(readBack.size() * sizeof(GLuint)), + readBack.data()); + EXPECT_EQ(readBack[0], kGreen) << "glGetBufferSubData did not see the imageStore"; + EXPECT_EQ(readBack[2], kRed) << "the imageLoad side of the same dispatch read the wrong texel"; + + const void* mapped = glMapBuffer(GL_TEXTURE_BUFFER, GL_READ_ONLY); + EXPECT_NE(mapped, nullptr) << "glMapBuffer(GL_READ_ONLY) on the texture's buffer failed"; + if (mapped != nullptr) { + GLuint mappedTexel0 = 0; + std::memcpy(&mappedTexel0, mapped, sizeof(mappedTexel0)); + EXPECT_EQ(mappedTexel0, kGreen) << "glMapBuffer did not see the imageStore"; + glUnmapBuffer(GL_TEXTURE_BUFFER); + } + + glBindImageTexture(0, 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_RGBA8); + glBindBuffer(GL_TEXTURE_BUFFER, 0); + glBindTexture(GL_TEXTURE_BUFFER, 0); + glUseProgram(0); + glDeleteProgram(program); + glDeleteTextures(1, &texture); + glDeleteBuffers(1, &buffer); + EXPECT_EQ(FirstGLError(), 0u); + } + } // namespace MGITest From f91857266f0f4e1ead2c6ebf80733539acbbbe94 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 17:50:31 -0400 Subject: [PATCH 3/3] [Fix, Test] (MG_Backend/DirectGLES, MG_Test): the tessellation stages never got the flat qualifier an ES linker compares across both sides of every integer interface --- MobileGL/MG_Backend/DirectGLES/Utils.cpp | 17 +++++ .../Backend/DirectGLES/EsslShaderPassTest.cpp | 68 +++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index 73deaa02..6251f10c 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -417,10 +417,27 @@ namespace MobileGL::MG_Backend::DirectGLES { result = std::regex_replace(result, pattern, "$1flat $2"); }; + // Every stage that has an integer interface at all, on BOTH sides. Interpolation is + // only ever consumed at a fragment input, so the qualifier is semantically inert on + // a tessellation or geometry interface - but an ES linker still compares the two + // sides of every interface and rejects a program whose producer says `flat` and + // whose consumer does not. Covering only the stages that "need" it left exactly two + // holes, and a program that used tessellation fell into both: + // vertex `flat out uint` -> tess-control `in uint` (producer flat, consumer not) + // tess-eval `out uint` -> geometry `flat in uint` (consumer flat, producer not) + // Adreno answers "output ... interpolation mismatch with other stage" and the whole + // program fails to link, which is a draw that silently paints nothing. + // + // Adding rather than stripping, because a fragment input's `flat` is load-bearing + // (ESSL forbids an interpolated integer) and would have to be put back for the last + // stage before the fragment shader anyway - so "everything integer is flat" is the + // one rule that is consistent no matter which stages a program happens to have. switch (shaderType) { case GL_VERTEX_SHADER: addFlatQualifier("out"); break; + case GL_TESS_CONTROL_SHADER: + case GL_TESS_EVALUATION_SHADER: case GL_GEOMETRY_SHADER: addFlatQualifier("in"); addFlatQualifier("out"); diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp index b0e3a487..09e124ab 100644 --- a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -16,6 +16,7 @@ #include using namespace MobileGL; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::ForceFlatIntegerVaryings; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITE_ALIAS_PREFIX; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemoveLayoutBinding; using MobileGL::MG_Backend::DirectGLES::PrgramImpl::SplitReadWriteImageUniforms; @@ -367,3 +368,70 @@ void main() {} << "an unrelated extension must survive untouched:\n" << out; EXPECT_EQ(CountOf(out, "GL_OES_texture_buffer"), 1u); } + +// Interpolation is only ever consumed at a fragment input, but an ES linker still compares the +// two sides of EVERY stage interface and rejects a program whose producer says `flat` and whose +// consumer does not. SPIRV-Cross prints `flat` on a vertex output and a geometry input of +// integer type and on nothing else, so a program with tessellation in the middle came out +// mismatched at both ends of the tessellator - "output vs_tcs_result interpolation mismatch +// with other stage" on Adreno, and a program that fails to link is a draw that paints nothing. +TEST(ForceFlatIntegerVaryingsTest, TessellationStagesGetTheQualifierOnBothSides) { + const String tessControl = R"(#version 320 es +layout(vertices = 1) out; +layout(location = 0) in uint vs_tcs_result[]; +layout(location = 0) out uint tcs_tes_result[1]; +void main() { tcs_tes_result[gl_InvocationID] = vs_tcs_result[gl_InvocationID]; } +)"; + const String control = ForceFlatIntegerVaryings(tessControl, GL_TESS_CONTROL_SHADER); + EXPECT_TRUE(Contains(control, "layout(location = 0) flat in uint vs_tcs_result[];")) << control; + EXPECT_TRUE(Contains(control, "layout(location = 0) flat out uint tcs_tes_result[1];")) << control; + + const String tessEval = R"(#version 320 es +layout(isolines, point_mode) in; +layout(location = 0) in uint tcs_tes_result[]; +layout(location = 0) out uint tes_gs_result; +void main() { tes_gs_result = tcs_tes_result[0]; } +)"; + const String eval = ForceFlatIntegerVaryings(tessEval, GL_TESS_EVALUATION_SHADER); + EXPECT_TRUE(Contains(eval, "layout(location = 0) flat in uint tcs_tes_result[];")) << eval; + EXPECT_TRUE(Contains(eval, "layout(location = 0) flat out uint tes_gs_result;")) << eval; +} + +// The two ends the tessellation stages have to meet: what a vertex shader and a geometry shader +// already emitted before this pass learned about tessellation at all. Pinned here so the two +// sides cannot drift apart again. +TEST(ForceFlatIntegerVaryingsTest, TheStagesAroundTessellationAreUnchanged) { + const String vertex = R"(#version 320 es +layout(location = 0) out uint vs_tcs_result; +void main() { vs_tcs_result = 1u; } +)"; + EXPECT_TRUE(Contains(ForceFlatIntegerVaryings(vertex, GL_VERTEX_SHADER), + "layout(location = 0) flat out uint vs_tcs_result;")); + + const String geometry = R"(#version 320 es +layout(points) in; +layout(triangle_strip, max_vertices = 4) out; +layout(location = 0) in uint tes_gs_result[1]; +layout(location = 0) out uint gs_fs_result; +void main() { gs_fs_result = tes_gs_result[0]; EmitVertex(); } +)"; + const String gs = ForceFlatIntegerVaryings(geometry, GL_GEOMETRY_SHADER); + EXPECT_TRUE(Contains(gs, "layout(location = 0) flat in uint tes_gs_result[1];")) << gs; + EXPECT_TRUE(Contains(gs, "layout(location = 0) flat out uint gs_fs_result;")) << gs; +} + +// Non-integer interfaces keep whatever interpolation they were given: adding `flat` to a float +// varying would turn a smoothly interpolated value into a per-provoking-vertex constant, which +// is a rendering change, not a linker one. +TEST(ForceFlatIntegerVaryingsTest, FloatVaryingsAreNotTouched) { + const String tessEval = R"(#version 320 es +layout(isolines, point_mode) in; +layout(location = 1) in vec2 tcs_tes_coord[]; +layout(location = 1) out vec2 tes_gs_coord; +void main() { tes_gs_coord = tcs_tes_coord[0]; } +)"; + const String out = ForceFlatIntegerVaryings(tessEval, GL_TESS_EVALUATION_SHADER); + EXPECT_TRUE(Contains(out, "layout(location = 1) in vec2 tcs_tes_coord[];")) << out; + EXPECT_TRUE(Contains(out, "layout(location = 1) out vec2 tes_gs_coord;")) << out; + EXPECT_EQ(CountOf(out, "flat"), 0u) << out; +}