From 239fc228fcdd24e300e4d03e397957d54bbdfd18 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 2 Oct 2026 13:34:16 -0400 Subject: [PATCH] [Fix] (DirectVulkan): retype gl_FragCoord access chains to Private when the y-flip pass redirects them to its copy --- .../DirectVulkan/Renderer/ProgramFactory.cpp | 102 ++++++++++++---- .../DirectVulkan/Renderer/ProgramFactory.h | 5 + .../MG_Test/ShaderTranspiler/CMakeLists.txt | 1 + .../ShaderTranspiler/FragCoordYFlipTest.cpp | 113 ++++++++++++++++++ 4 files changed, 195 insertions(+), 26 deletions(-) create mode 100644 MobileGL/MG_Test/ShaderTranspiler/FragCoordYFlipTest.cpp diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 5228cf01..9ca6539e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -1313,6 +1313,30 @@ namespace MobileGL::MG_Backend::DirectVulkan { } }); if (!ok) return false; + // An access chain into the builtin (gl_FragCoord.y is one) carries an Input + // pointer type. Based on the Private copy it has to carry a Private one, and so + // does every chain built on top of it - otherwise the module hands the driver a + // pointer whose storage class disagrees with its base, which is invalid SPIR-V + // and reads an undefined value. Plan every retype before touching anything, so a + // shape this pass does not model declines the whole rewrite. + Vector> retypes; + Vector chains; + for (Instruction* user : users) { + if (IsAccessChain(user->opcode())) chains.push_back(user); + } + while (!chains.empty()) { + Instruction* chain = chains.back(); + chains.pop_back(); + const Uint32 privateType = PrivatePointerTypeFor(chain->type_id()); + if (privateType == 0) return false; + retypes.emplace_back(chain, privateType); + context()->get_def_use_mgr()->ForEachUser(chain, [&](Instruction* next) { + if (IsAccessChain(next->opcode()) && next->NumInOperands() > 0 && + next->GetSingleWordInOperand(0) == chain->result_id()) { + chains.push_back(next); + } + }); + } for (Instruction* user : users) { for (Uint32 i = 0; i < user->NumInOperands(); ++i) { auto& operand = user->GetInOperand(i); @@ -1323,9 +1347,32 @@ namespace MobileGL::MG_Backend::DirectVulkan { } context()->AnalyzeUses(user); } + for (auto& [chain, privateType] : retypes) { + chain->SetResultType(privateType); + context()->AnalyzeUses(chain); + } return true; } + static Bool IsAccessChain(spv::Op opcode) { + return opcode == spv::Op::OpAccessChain || opcode == spv::Op::OpInBoundsAccessChain || + opcode == spv::Op::OpPtrAccessChain || opcode == spv::Op::OpInBoundsPtrAccessChain; + } + + // The Private-storage twin of an Input pointer type, or 0 when `pointerTypeId` is not + // an Input pointer (then the chain is not the shape this pass rewrites). + Uint32 PrivatePointerTypeFor(Uint32 pointerTypeId) { + using namespace spvtools::opt; + Instruction* pointerType = context()->get_def_use_mgr()->GetDef(pointerTypeId); + if (!pointerType || pointerType->opcode() != spv::Op::OpTypePointer) return 0; + if (static_cast(pointerType->GetSingleWordInOperand(0)) != + spv::StorageClass::Input) { + return 0; + } + return context()->get_type_mgr()->FindPointerToType(pointerType->GetSingleWordInOperand(1), + spv::StorageClass::Private); + } + Uint32 m_framebufferHeight = 0; }; @@ -1861,35 +1908,38 @@ namespace MobileGL::MG_Backend::DirectVulkan { return spvtools::Optimizer::PassToken(MakeUnique(transformFlags)); } - Bool TransformSpirvForFragCoordYFlip(const Vector& input, Vector& output, - Uint32 framebufferHeight) { - if (input.empty()) { - output.clear(); - return true; - } - if (framebufferHeight == 0) { - output = input; - return true; - } + } // namespace - spvtools::Optimizer optimizer(SPV_ENV_VULKAN_1_3); - spvtools::OptimizerOptions options; - options.set_run_validator(false); // see TransformSpirvForExplicitLod0Sampling - optimizer.SetMessageConsumer([](spv_message_level_t, const char*, const spv_position_t&, - const char* message) { - MGLOG_E("Vulkan: fragcoord y-flip pass: %s", message != nullptr ? message : ""); - }); - optimizer.RegisterPass( - spvtools::Optimizer::PassToken(MakeUnique(framebufferHeight))); - - const Bool success = optimizer.Run(input.data(), input.size(), &output, options); - if (!success) { - MGLOG_E("Vulkan: failed to run the gl_FragCoord y-flip pass; keeping the original module"); - output = input; - } - return success; + Bool TransformSpirvForFragCoordYFlip(const Vector& input, Vector& output, + Uint32 framebufferHeight) { + if (input.empty()) { + output.clear(); + return true; + } + if (framebufferHeight == 0) { + output = input; + return true; } + spvtools::Optimizer optimizer(SPV_ENV_VULKAN_1_3); + spvtools::OptimizerOptions options; + options.set_run_validator(false); // see TransformSpirvForExplicitLod0Sampling + optimizer.SetMessageConsumer([](spv_message_level_t, const char*, const spv_position_t&, + const char* message) { + MGLOG_E("Vulkan: fragcoord y-flip pass: %s", message != nullptr ? message : ""); + }); + optimizer.RegisterPass( + spvtools::Optimizer::PassToken(MakeUnique(framebufferHeight))); + + const Bool success = optimizer.Run(input.data(), input.size(), &output, options); + if (!success) { + MGLOG_E("Vulkan: failed to run the gl_FragCoord y-flip pass; keeping the original module"); + output = input; + } + return success; + } + + namespace { Bool TransformSpirvForXfbCapture(const Vector& input, Vector& output, const MagmaProgramSource& program) { if (input.empty()) { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index f5717b79..5858a485 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -658,4 +658,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { UnorderedMap m_passthroughTessControlStages; static inline thread_local XXH64_state_t* m_hashState = XXH64_createState(); }; + + // The FragCoordYFlip variant's module transform: every read of gl_FragCoord goes through a + // Private copy whose y is (framebufferHeight - y). Fails open (output = input, false). Exposed + // so a unit case can validate the bytes it hands the driver. + Bool TransformSpirvForFragCoordYFlip(const Vector& input, Vector& output, Uint32 framebufferHeight); } // namespace MobileGL::MG_Backend::DirectVulkan diff --git a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt index 6ee4482e..705182da 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt +++ b/MobileGL/MG_Test/ShaderTranspiler/CMakeLists.txt @@ -19,6 +19,7 @@ add_executable( WidenImageFormatsTest.cpp GlslangCaptureTest.cpp DemotePointSizeTest.cpp + FragCoordYFlipTest.cpp ) target_include_directories(SpirvPassTest PRIVATE diff --git a/MobileGL/MG_Test/ShaderTranspiler/FragCoordYFlipTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/FragCoordYFlipTest.cpp new file mode 100644 index 00000000..15135887 --- /dev/null +++ b/MobileGL/MG_Test/ShaderTranspiler/FragCoordYFlipTest.cpp @@ -0,0 +1,113 @@ +// MobileGL - MobileGL/MG_Test/ShaderTranspiler/FragCoordYFlipTest.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +// Magma's FragCoordYFlip variant redirects every read of gl_FragCoord to a Private copy. A +// single-component read (gl_FragCoord.y) is an access chain whose result type is an INPUT +// pointer; left that way on a Private base it is invalid SPIR-V that the driver still accepts and +// reads garbage through. The module must validate, and the chain must point at the copy. + +#include + +#include "Includes.h" +#include "Init.h" +#include +#include +#include + +#include "spirv-tools/libspirv.hpp" + +using namespace MobileGL; +using MobileGL::MG_Util::ShaderTranspiler::ShaderCompiler; + +namespace { + Vector> CompileProgram(const char* vertex, const char* fragment) { + using namespace MG_Util::ShaderTranspiler; + Vector> shaders; + Vector types; + for (const auto& [stage, source] : Vector>{{GL_VERTEX_SHADER, vertex}, + {GL_FRAGMENT_SHADER, fragment}}) { + ShaderAttrib attrib{.shaderType = stage, .sourceStr = source}; + auto shader = ShaderCompiler::CompileShader(attrib); + EXPECT_TRUE(shader) << (shader ? String{} : shader.error().log); + if (!shader) return {}; + shaders.push_back(shader.value()); + types.push_back(stage); + } + ProgramAttrib programAttrib{.shaders = shaders}; + auto program = ShaderCompiler::LinkProgram(programAttrib); + EXPECT_TRUE(program) << (program ? String{} : program.error().log); + if (!program) return {}; + ProgramBinaryAttrib binaryAttrib{.shaderTypes = types, .program = *program.value()}; + auto binary = ShaderCompiler::GetSpirvBinaryFromProgram(binaryAttrib); + EXPECT_TRUE(binary) << (binary ? String{} : binary.error().log); + if (!binary) return {}; + Vector> modules = Move(binary.value()); + for (auto& module : modules) { + EXPECT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(module, module, true, true)); + } + return modules; + } + + String Disassemble(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + String text; + EXPECT_TRUE(tools.Disassemble(spirv, &text)); + return text; + } + + String ValidationError(const Vector& spirv) { + spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_1); + String error; + tools.SetMessageConsumer([&](spv_message_level_t, const char*, const spv_position_t&, + const char* message) { error += message != nullptr ? message : ""; }); + return tools.Validate(spirv) ? String{} : (error.empty() ? String{"invalid"} : error); + } + + constexpr const char* kVertex = R"(#version 330 core +layout(location = 0) in vec2 position; +void main() { gl_Position = vec4(position, 0.0, 1.0); } +)"; + + void ExpectFlippedModuleValid(const char* fragment) { + MobileGL::Initialize(); + auto modules = CompileProgram(kVertex, fragment); + ASSERT_EQ(modules.size(), 2u); + const Vector& input = modules[1]; + ASSERT_EQ(ValidationError(input), String{}); + + Vector flipped; + ASSERT_TRUE(MG_Backend::DirectVulkan::TransformSpirvForFragCoordYFlip(input, flipped, 720)); + EXPECT_EQ(ValidationError(flipped), String{}) << Disassemble(flipped); + // The flip really happened: the height is subtracted, and no chain still reads the + // builtin's Input storage. + const String text = Disassemble(flipped); + EXPECT_NE(text.find("OpFSub"), String::npos) << text; + EXPECT_EQ(text.find("OpAccessChain %_ptr_Input_float"), String::npos) << text; + } +} // namespace + +TEST(FragCoordYFlipTest, SingleComponentReadStaysValid) { + ExpectFlippedModuleValid(R"(#version 330 core +out vec4 color; +void main() { color = vec4(gl_FragCoord.y / 720.0, 0.0, 0.0, 1.0); } +)"); +} + +TEST(FragCoordYFlipTest, ComponentAndWholeVectorReadsStayValid) { + ExpectFlippedModuleValid(R"(#version 330 core +out vec4 color; +uniform float limit; +void main() { + float acc = 0.0; + for (int i = 0; i < 4; ++i) { + if (gl_FragCoord.x > limit) acc += gl_FragCoord.y; + } + color = vec4(gl_FragCoord.xy, acc, 1.0); +} +)"); +}