diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 894a0103..ad8718c0 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -7341,11 +7341,19 @@ namespace MobileGL::MG_Backend::DirectGLES { // * this program has a stage that can hit it - a located block between a // vertex and a fragment stage works on the affected driver, so a program // with neither tessellation nor geometry keeps its ESSL byte for byte; - // * for THIS stage and THIS direction, the other end of the interface is in - // this same program. In a separate-shader-objects pipeline it is not, and - // the location is the only thing matching the two programs across - the - // identical reason the rename plan above tests producer/consumer presence. - // The direction tests reuse that plan's answers rather than recomputing them. + // * for THIS stage and THIS direction, this program HAS a stage on that side + // of it. That is the same test the rename plan above makes, and the same + // approximation: it asks "is some stage of this program earlier/later than + // me", not "is the exact partner of every one of my blocks here". The two + // coincide for every program MobileGL builds, because a separable pipeline + // is flattened into one composite carrying every stage that has a shader + // (GLContext::GetProgramForDraw) and a program bound with glUseProgram has + // no partner program at all - so a stage set with a gap in it does not + // arise. Should one ever arise, this must become the nearest-stage + // resolution the rename plan computes, or the two ends of the gap would + // disagree about the qualifier. + // The direction tests deliberately mirror that plan rather than inventing a + // second rule for the same question. Bool stripInputBlockLocations = false; Bool stripOutputBlockLocations = false; if (ioBlockLocationStripArmed && stagePipelineIndices[index] >= 0) { diff --git a/MobileGL/MG_Test/ShaderTranspiler/StripIoBlockLocationsTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/StripIoBlockLocationsTest.cpp index 3bae499b..6b45b452 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/StripIoBlockLocationsTest.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/StripIoBlockLocationsTest.cpp @@ -81,6 +81,31 @@ void main() tes_gs_result = tcs_tes_result[0]; output_block.tes_gs_variable = input_block[0].tcs_tes_variable; } +)"; + + // The OTHER place a block's location can live. When the application locates the MEMBERS + // rather than the block, glslang emits one OpMemberDecorate Location per member and + // NOTHING on the variable - and SPIRV-Cross then suppresses the block-level qualifier and + // prints the member ones instead. A strip that only looked at the variable would find + // nothing to remove here, report "unchanged", and leave the emitted ESSL carrying exactly + // the located block the driver drops the payload for. + const char* kMemberLocatedTessEvalSource = R"(#version 450 core +layout(isolines, point_mode) in; + +in TCSOutputBlock { + layout(location = 4) vec4 tcs_tes_variable; + layout(location = 5) vec4 tcs_tes_second; +} input_block[]; +out TESOutputBlock { + layout(location = 6) vec4 tes_gs_variable; + layout(location = 7) vec4 tes_gs_second; +} output_block; + +void main() +{ + output_block.tes_gs_variable = input_block[0].tcs_tes_variable; + output_block.tes_gs_second = input_block[0].tcs_tes_second; +} )"; // A stage with no interface block at all: the pass must leave its located varyings alone @@ -183,6 +208,58 @@ TEST_F(StripIoBlockLocationsTest, StripsOnlyTheDirectionTheCallerArmed) { EXPECT_EQ(afterOutputOnly.find(") out TESOutputBlock"), String::npos) << afterOutputOnly; } +// The regression guard for the shape a variable-only strip walks straight past. +TEST_F(StripIoBlockLocationsTest, DropsLocationsTheApplicationPutOnTheBlockMembers) { + const Vector input = CompileToSpirv(GL_TESS_EVALUATION_SHADER, kMemberLocatedTessEvalSource); + ASSERT_FALSE(input.empty()); + + // The defect, pinned first: SPIRV-Cross prints the member locations, and there is no + // block-level qualifier for a variable-level strip to find. + const String before = Transpile(input); + EXPECT_NE(before.find("layout(location = 4)"), String::npos) << before; + EXPECT_NE(before.find("layout(location = 6)"), String::npos) << before; + + bool strippedAny = false; + Vector output; + ASSERT_TRUE(ShaderCompiler::StripIoBlockLocationsForEssl(input, true, true, strippedAny, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_TRUE(strippedAny) << "the member-located block was passed by, and reporting no change " + "makes the caller decline the module and say nothing about it"; + + const String after = Transpile(output); + EXPECT_EQ(CountOf(after, "layout(location"), 0u) + << "a member location survived, so the emitted block is still the shape the driver " + "drops the payload for:\n" + << after; + // The interface still has to be matchable: same blocks, same members, same order. + EXPECT_NE(after.find("TCSOutputBlock"), String::npos) << after; + EXPECT_NE(after.find("TESOutputBlock"), String::npos) << after; + EXPECT_LT(after.find("tcs_tes_variable"), after.find("tcs_tes_second")) << after; + EXPECT_LT(after.find("tes_gs_variable"), after.find("tes_gs_second")) << after; +} + +// ...and the same shape with only ONE direction armed. The member decorations belong to the +// TYPE, so the unarmed block's must survive - it is matched, in another program, by exactly +// those numbers. +TEST_F(StripIoBlockLocationsTest, KeepsMemberLocationsOnTheDirectionTheCallerDidNotArm) { + const Vector input = CompileToSpirv(GL_TESS_EVALUATION_SHADER, kMemberLocatedTessEvalSource); + ASSERT_FALSE(input.empty()); + + bool strippedAny = false; + Vector output; + ASSERT_TRUE(ShaderCompiler::StripIoBlockLocationsForEssl(input, true, false, strippedAny, output, true)); + ASSERT_FALSE(output.empty()); + EXPECT_TRUE(strippedAny); + + const String after = Transpile(output); + EXPECT_EQ(after.find("layout(location = 4)"), String::npos) << after; + EXPECT_EQ(after.find("layout(location = 5)"), String::npos) << after; + EXPECT_NE(after.find("layout(location = 6)"), String::npos) + << "the produced block lost its member locations even though its consumer is elsewhere:\n" + << after; + EXPECT_NE(after.find("layout(location = 7)"), String::npos) << after; +} + TEST_F(StripIoBlockLocationsTest, ReportsNoChangeForAStageWithoutInterfaceBlocks) { const Vector input = CompileToSpirv(GL_TESS_EVALUATION_SHADER, kNoBlockTessEvalSource); ASSERT_FALSE(input.empty()); diff --git a/MobileGL/MG_Util/SelfTest/DriverBugProbes.cpp b/MobileGL/MG_Util/SelfTest/DriverBugProbes.cpp index c1ee759d..575b3633 100644 --- a/MobileGL/MG_Util/SelfTest/DriverBugProbes.cpp +++ b/MobileGL/MG_Util/SelfTest/DriverBugProbes.cpp @@ -8,6 +8,7 @@ #include "DriverBugProbes.h" +#include #include #include @@ -1828,11 +1829,19 @@ namespace MobileGL::MG_Util::SelfTest { SavedState saved; Save(gl, saved); + // The colour mask is not in SavedState - no other probe touches it - so this one saves + // and puts back its own. It has to be forced open: a masked channel would read back as + // zero and turn a healthy driver into a "payload lost" verdict. + GLboolean savedColorMask[4] = {GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE}; + const Bool canMaskColor = gl.glColorMask != nullptr && gl.glGetBooleanv != nullptr; + if (canMaskColor) { + gl.glGetBooleanv(GL_COLOR_WRITEMASK, savedColorMask); + gl.glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + } GLuint vao = 0; gl.glGenVertexArrays(1, &vao); gl.glBindVertexArray(vao); PrepareForProbeDraw(gl); - if (gl.glColorMask != nullptr) gl.glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); // THE CONTROL, and it runs first: the identical three-stage program with no location on // the blocks. If THAT cannot carry the payload, this driver's problem is not the @@ -1863,6 +1872,9 @@ namespace MobileGL::MG_Util::SelfTest { gl.glBindVertexArray(0); gl.glDeleteVertexArrays(1, &vao); } + if (canMaskColor) { + gl.glColorMask(savedColorMask[0], savedColorMask[1], savedColorMask[2], savedColorMask[3]); + } Restore(gl, saved); Drain(gl); return measurement; @@ -2015,13 +2027,27 @@ namespace MobileGL::MG_Util::SelfTest { "vertex+fragment program is still emitted as the application wrote it" : ". A located block between a VERTEX and a FRAGMENT stage is delivered " "correctly on the same driver, which is what scopes the repair"; - detail += - ". MobileGL emits a tessellation/geometry program's interface blocks with no " - "location qualifier at all (StripIoBlockLocationsPass) and lets ES match them by " - "block name and member sequence, which it does; the locations were invented by " - "the cross-stage IO resolver rather than written by the application"; + // The repair can be switched off from the environment, and a report that said + // "Fixed" while the strip was disabled would be describing a build nobody is + // running. The verdict follows what this process will actually do, not what the + // code is capable of. + const Bool repairDisabled = + MG_Config::Features.EsprytUnlocatedIoBlocks == MG_Config::QuirkOverride::ForceOff; + if (repairDisabled) { + detail += + ". THE REPAIR IS DISABLED in this process: MOBILEGL_ESPRYT_UNLOCATED_IO_BLOCKS " + "is set to force located blocks ON, so DirectGLES emits the location " + "qualifier the driver cannot honour and the payload is lost. Unset the " + "variable to get the repair back"; + } else { + detail += + ". MobileGL emits a tessellation/geometry program's interface blocks with no " + "location qualifier at all (StripIoBlockLocationsPass) and lets ES match them " + "by block name and member sequence, which it does; the locations were invented " + "by the cross-stage IO resolver rather than written by the application"; + } return DriverBugFinding{"Located inter-stage interface blocks carry no payload", - measurement.alsoAffectsVertexToFragment + (repairDisabled || measurement.alsoAffectsVertexToFragment) ? DriverBugVerdict::Unfixable : DriverBugVerdict::Fixed, Move(detail)}; diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.cpp index 4cb7e681..ef940a2b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.cpp @@ -54,27 +54,24 @@ namespace MobileGL { return blockStructIds; } - // True when `variable` is an Input/Output interface block of the direction the - // caller armed. Tessellation and geometry interfaces are arrays of the block - // struct, so array levels are unwrapped before the struct is recognised. - Bool IsArmedInterfaceBlock(IRContext* irContext, Instruction& variable, - const std::unordered_set& blockStructIds, - Bool stripInputBlocks, Bool stripOutputBlocks) { - if (variable.opcode() != spv::Op::OpVariable) return false; + // The interface-block struct an Input/Output variable declares, or 0 when the + // variable is not one. Tessellation and geometry interfaces are arrays of the + // block struct, so array levels are unwrapped before the struct is recognised. + uint32_t GetInterfaceBlockStructId(IRContext* irContext, Instruction& variable, + const std::unordered_set& blockStructIds, + spv::StorageClass& outStorageClass) { + if (variable.opcode() != spv::Op::OpVariable) return 0; const auto storageClass = static_cast(variable.GetSingleWordInOperand(0)); - if (storageClass == spv::StorageClass::Input) { - if (!stripInputBlocks) return false; - } else if (storageClass == spv::StorageClass::Output) { - if (!stripOutputBlocks) return false; - } else { - return false; + if (storageClass != spv::StorageClass::Input && + storageClass != spv::StorageClass::Output) { + return 0; } auto* defUseMgr = irContext->get_def_use_mgr(); Instruction* pointerType = defUseMgr->GetDef(variable.type_id()); if (pointerType == nullptr || pointerType->opcode() != spv::Op::OpTypePointer) { - return false; + return 0; } uint32_t pointeeId = pointerType->GetSingleWordInOperand(1); Instruction* pointee = defUseMgr->GetDef(pointeeId); @@ -83,8 +80,16 @@ namespace MobileGL { pointeeId = pointee->GetSingleWordInOperand(0); pointee = defUseMgr->GetDef(pointeeId); } - if (pointee == nullptr || pointee->opcode() != spv::Op::OpTypeStruct) return false; - return blockStructIds.find(pointeeId) != blockStructIds.end(); + if (pointee == nullptr || pointee->opcode() != spv::Op::OpTypeStruct) return 0; + if (blockStructIds.find(pointeeId) == blockStructIds.end()) return 0; + + outStorageClass = storageClass; + return pointeeId; + } + + Bool DirectionIsArmed(spv::StorageClass storageClass, Bool stripInputBlocks, + Bool stripOutputBlocks) { + return storageClass == spv::StorageClass::Input ? stripInputBlocks : stripOutputBlocks; } } // namespace @@ -96,36 +101,74 @@ namespace MobileGL { const std::unordered_set blockStructIds = CollectUserBlockStructIds(irContext); if (blockStructIds.empty()) return Status::SuccessWithoutChange; - // The variable ids to strip, resolved BEFORE anything is killed: the walk below - // deletes annotations, and deciding what to delete while deleting reads a list - // that is being mutated underneath it. + // What to strip, resolved BEFORE anything is killed: the walk below deletes + // annotations, and deciding what to delete while deleting reads a list that is + // being mutated underneath it. + // + // BOTH LEVELS, because a block carries its location at exactly one of them and + // which one is not the caller's choice. When the location came from the + // cross-stage IO resolver (or from `layout(location=) out Blk {...}`) glslang + // puts it on the VARIABLE; when the application located the members instead + // (`out Blk { layout(location = 4) vec4 v; }`) it puts one OpMemberDecorate per + // member and NOTHING on the variable - and SPIRV-Cross then suppresses the + // block-level qualifier and prints the member ones instead + // (spirv_glsl.cpp:1444 and :2037-2045). Stripping only the variable level would + // leave that second shape emitting exactly the located block this driver drops + // the payload for, and - because there was no variable decoration to remove - + // would report nothing stripped, so the caller would decline the module and + // nothing would say the repair had passed the shader by. std::unordered_set armedVariableIds; + std::unordered_set armedStructIds; + // Block structs reached by an interface variable whose direction is NOT armed. + // A struct in here is left alone even if some armed variable also reaches it: + // member decorations belong to the TYPE, so stripping them would take the + // qualifier off the unarmed side too - the one whose other end is in a + // different program and is matched by exactly that number. + std::unordered_set unarmedStructIds; for (Instruction& variable : irContext->module()->types_values()) { - if (IsArmedInterfaceBlock(irContext, variable, blockStructIds, m_stripInputBlocks, - m_stripOutputBlocks)) { + spv::StorageClass storageClass = spv::StorageClass::Input; + const uint32_t structId = + GetInterfaceBlockStructId(irContext, variable, blockStructIds, storageClass); + if (structId == 0) continue; + if (DirectionIsArmed(storageClass, m_stripInputBlocks, m_stripOutputBlocks)) { armedVariableIds.insert(variable.result_id()); + armedStructIds.insert(structId); + } else { + unarmedStructIds.insert(structId); } } + for (const uint32_t unarmedStructId : unarmedStructIds) { + armedStructIds.erase(unarmedStructId); + } if (armedVariableIds.empty()) return Status::SuccessWithoutChange; + // Component travels with Location and is meaningless without it. Leaving one + // behind is not merely untidy: for an ES target SPIRV-Cross THROWS on a block + // member's Component (spirv_glsl.cpp:1447-1460) rather than printing it, which + // costs the whole stage. + const auto isLocationOrComponent = [](uint32_t decoration) { + return static_cast(decoration) == spv::Decoration::Location || + static_cast(decoration) == spv::Decoration::Component; + }; + std::vector toKill; for (Instruction& annotation : irContext->module()->annotations()) { - if (annotation.opcode() != spv::Op::OpDecorate) continue; - const auto decoration = - static_cast(annotation.GetSingleWordInOperand(1)); - // Component travels with Location and is meaningless without it; a block - // whose Location is gone and whose Component survives would be a shader - // SPIRV-Cross prints `layout(component = N)` for on its own, which ESSL has - // no spelling for at all. - if (decoration != spv::Decoration::Location && - decoration != spv::Decoration::Component) { - continue; + if (annotation.opcode() == spv::Op::OpDecorate) { + if (!isLocationOrComponent(annotation.GetSingleWordInOperand(1))) continue; + if (armedVariableIds.find(annotation.GetSingleWordInOperand(0)) == + armedVariableIds.end()) { + continue; + } + toKill.push_back(&annotation); + } else if (annotation.opcode() == spv::Op::OpMemberDecorate) { + // OpMemberDecorate ... + if (!isLocationOrComponent(annotation.GetSingleWordInOperand(2))) continue; + if (armedStructIds.find(annotation.GetSingleWordInOperand(0)) == + armedStructIds.end()) { + continue; + } + toKill.push_back(&annotation); } - if (armedVariableIds.find(annotation.GetSingleWordInOperand(0)) == - armedVariableIds.end()) { - continue; - } - toKill.push_back(&annotation); } for (Instruction* inst : toKill) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.h index 14789373..9b5f102a 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.h @@ -46,6 +46,16 @@ namespace MobileGL { // and so do vertex attributes and fragment outputs, which are never blocks. // Builtin blocks (gl_PerVertex) are skipped; they carry no Location anyway. // + // BOTH DECORATION LEVELS, because a block carries its location at exactly one of + // them: on the VARIABLE when the cross-stage IO resolver assigned it (or the + // application wrote `layout(location=) out Blk {...}`), and on the MEMBERS when the + // application located those instead - in which case glslang puts nothing on the + // variable at all and SPIRV-Cross suppresses the block-level qualifier in favour of + // the member ones. A variable-only strip would silently pass that second shape by. + // A struct reached by an interface variable whose direction is NOT armed keeps its + // member decorations: they belong to the type, and taking them off would strip the + // unarmed side too. + // // The two directions are armed SEPARATELY by the caller, because an interface // whose other end lives in a DIFFERENT program (a separable program pipeline) // must keep its location: that is the only thing matching it there, and the other