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] (Review): strip a block's member-level locations too, restore the probe's colour mask, and let the POST verdict follow the override

Browse Source
This commit is contained in:
swung0x48
2026-08-27 20:15:31 -04:00
parent eab622388f
commit ad28d2b744
5 changed files with 212 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Backend/DirectGLES/Managers.cpp
+13 -5
View File
@@ -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) {
MobileGL/MG_Test/ShaderTranspiler/StripIoBlockLocationsTest.cpp
+77
View File
@@ -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<Uint32> 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<Uint32> 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<Uint32> input = CompileToSpirv(GL_TESS_EVALUATION_SHADER, kMemberLocatedTessEvalSource);
ASSERT_FALSE(input.empty());
bool strippedAny = false;
Vector<Uint32> 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<Uint32> input = CompileToSpirv(GL_TESS_EVALUATION_SHADER, kNoBlockTessEvalSource);
ASSERT_FALSE(input.empty());
MobileGL/MG_Util/SelfTest/DriverBugProbes.cpp
+33 -7
View File
@@ -8,6 +8,7 @@
#include "DriverBugProbes.h"
#include <Config.h>
#include <MG_Util/Debug/Log.h>
#include <cstring>
@@ -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)};
MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.cpp
+79 -36
View File
@@ -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<uint32_t>& 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<uint32_t>& blockStructIds,
spv::StorageClass& outStorageClass) {
if (variable.opcode() != spv::Op::OpVariable) return 0;
const auto storageClass =
static_cast<spv::StorageClass>(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<uint32_t> 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<uint32_t> armedVariableIds;
std::unordered_set<uint32_t> 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<uint32_t> 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<spv::Decoration>(decoration) == spv::Decoration::Location ||
static_cast<spv::Decoration>(decoration) == spv::Decoration::Component;
};
std::vector<Instruction*> toKill;
for (Instruction& annotation : irContext->module()->annotations()) {
if (annotation.opcode() != spv::Op::OpDecorate) continue;
const auto decoration =
static_cast<spv::Decoration>(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 <struct> <member> <decoration> ...
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) {
MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/StripIoBlockLocationsPass.h
+10
View File
@@ -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