Skip to content

Commit 7bc3669

Browse files
hzqsthzqst
committed
Fix issue 3: OpName-based lookup is ambiguous and may target the wrong variable.
1 parent 6364a6c commit 7bc3669

File tree

1 file changed

+76
-57
lines changed

1 file changed

+76
-57
lines changed

‎Graphics/ShaderTools/src/ConvertUBOToPushConstant.cpp‎

Lines changed: 76 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -77,113 +77,132 @@ class ConvertUBOToPushConstantPass : public spvtools::opt::Pass
7777
{
7878
bool modified = false;
7979

80-
// Find the ID that matches the block name by searching OpName instructions
81-
// This could be either a variable ID or a type ID (struct type)
82-
uint32_t named_id = 0;
80+
// Collect all IDs that match the block name by searching OpName instructions.
81+
// Multiple OpName instructions may have the same name, so we need to check all of them
82+
// to find the one that refers to a UniformBuffer (Uniform storage class + Block decoration).
83+
std::vector<uint32_t> candidate_ids;
8384
for (auto& debug_inst : context()->module()->debugs2())
8485
{
8586
if (debug_inst.opcode() == spv::Op::OpName &&
8687
debug_inst.GetOperand(1).AsString() == m_BlockName)
8788
{
88-
named_id = debug_inst.GetOperand(0).AsId();
89-
break;
89+
candidate_ids.push_back(debug_inst.GetOperand(0).AsId());
9090
}
9191
}
9292

93-
if (named_id == 0)
93+
if (candidate_ids.empty())
9494
{
9595
// Block name not found
9696
return Status::SuccessWithoutChange;
9797
}
9898

99-
// Check if the named_id is a variable or a type
99+
// Try each candidate ID to find a UniformBuffer
100100
spvtools::opt::Instruction* target_var = nullptr;
101-
spvtools::opt::Instruction* named_inst = get_def_use_mgr()->GetDef(named_id);
102-
103-
if (named_inst == nullptr)
104-
{
105-
return Status::SuccessWithoutChange;
106-
}
107-
108-
if (named_inst->opcode() == spv::Op::OpVariable)
101+
for (uint32_t named_id : candidate_ids)
109102
{
110-
// The name refers directly to a variable
111-
target_var = named_inst;
112-
}
113-
else if (named_inst->opcode() == spv::Op::OpTypeStruct)
114-
{
115-
// The name refers to a struct type, we need to find the variable
116-
// that uses a pointer to this struct type with Uniform storage class
117-
uint32_t struct_type_id = named_id;
103+
spvtools::opt::Instruction* named_inst = get_def_use_mgr()->GetDef(named_id);
104+
if (named_inst == nullptr)
105+
{
106+
continue;
107+
}
118108

119-
// Search for a variable that points to this struct type with Uniform storage class
120-
for (auto& inst : context()->types_values())
109+
if (named_inst->opcode() == spv::Op::OpVariable)
121110
{
122-
if (inst.opcode() != spv::Op::OpVariable)
111+
// The name refers directly to a variable - check if it's a UniformBuffer
112+
spvtools::opt::Instruction* var_inst = named_inst;
113+
114+
// Get the pointer type of the variable
115+
spvtools::opt::Instruction* ptr_type_inst = get_def_use_mgr()->GetDef(var_inst->type_id());
116+
if (ptr_type_inst == nullptr || ptr_type_inst->opcode() != spv::Op::OpTypePointer)
123117
{
124118
continue;
125119
}
126120

127-
// Get the pointer type of this variable
128-
spvtools::opt::Instruction* ptr_type = get_def_use_mgr()->GetDef(inst.type_id());
129-
if (ptr_type == nullptr || ptr_type->opcode() != spv::Op::OpTypePointer)
121+
// Check if the storage class is Uniform
122+
spv::StorageClass storage_class =
123+
static_cast<spv::StorageClass>(ptr_type_inst->GetSingleWordInOperand(0));
124+
if (storage_class != spv::StorageClass::Uniform)
130125
{
131126
continue;
132127
}
133128

134-
// Check storage class is Uniform
135-
spv::StorageClass sc = static_cast<spv::StorageClass>(
136-
ptr_type->GetSingleWordInOperand(0));
137-
if (sc != spv::StorageClass::Uniform)
129+
// Get the pointee type ID and verify it has Block decoration
130+
uint32_t pointee_type_id = ptr_type_inst->GetSingleWordInOperand(1);
131+
if (HasBlockDecoration(pointee_type_id))
138132
{
139-
continue;
133+
// Found a UniformBuffer!
134+
target_var = var_inst;
135+
break;
136+
}
137+
}
138+
else if (named_inst->opcode() == spv::Op::OpTypeStruct)
139+
{
140+
// The name refers to a struct type, we need to find the variable
141+
// that uses a pointer to this struct type with Uniform storage class
142+
uint32_t struct_type_id = named_id;
143+
144+
// Search for a variable that points to this struct type with Uniform storage class
145+
for (auto& inst : context()->types_values())
146+
{
147+
if (inst.opcode() != spv::Op::OpVariable)
148+
{
149+
continue;
150+
}
151+
152+
// Get the pointer type of this variable
153+
spvtools::opt::Instruction* ptr_type = get_def_use_mgr()->GetDef(inst.type_id());
154+
if (ptr_type == nullptr || ptr_type->opcode() != spv::Op::OpTypePointer)
155+
{
156+
continue;
157+
}
158+
159+
// Check storage class is Uniform
160+
spv::StorageClass sc = static_cast<spv::StorageClass>(
161+
ptr_type->GetSingleWordInOperand(0));
162+
if (sc != spv::StorageClass::Uniform)
163+
{
164+
continue;
165+
}
166+
167+
// Check if the pointee type is our struct type
168+
uint32_t pointee_type_id = ptr_type->GetSingleWordInOperand(1);
169+
if (pointee_type_id == struct_type_id)
170+
{
171+
// Verify it has Block decoration
172+
if (HasBlockDecoration(pointee_type_id))
173+
{
174+
// Found a UniformBuffer!
175+
target_var = &inst;
176+
break;
177+
}
178+
}
140179
}
141180

142-
// Check if the pointee type is our struct type
143-
uint32_t pointee_type_id = ptr_type->GetSingleWordInOperand(1);
144-
if (pointee_type_id == struct_type_id)
181+
if (target_var != nullptr)
145182
{
146-
target_var = &inst;
147183
break;
148184
}
149185
}
150186
}
151187

152188
if (target_var == nullptr)
153189
{
154-
// Variable not found
190+
// No UniformBuffer found with the given block name
155191
return Status::SuccessWithoutChange;
156192
}
157193

158194
uint32_t target_var_id = target_var->result_id();
159195

160-
// Get the pointer type of the variable
196+
// Get the pointer type of the variable (we already verified it above, but get it again for consistency)
161197
spvtools::opt::Instruction* ptr_type_inst = get_def_use_mgr()->GetDef(target_var->type_id());
162198
if (ptr_type_inst == nullptr || ptr_type_inst->opcode() != spv::Op::OpTypePointer)
163199
{
164200
return Status::SuccessWithoutChange;
165201
}
166202

167-
// Check if the storage class is Uniform
168-
spv::StorageClass storage_class =
169-
static_cast<spv::StorageClass>(ptr_type_inst->GetSingleWordInOperand(0));
170-
if (storage_class != spv::StorageClass::Uniform)
171-
{
172-
// Not a uniform buffer, nothing to do
173-
return Status::SuccessWithoutChange;
174-
}
175-
176203
// Get the pointee type ID
177204
uint32_t pointee_type_id = ptr_type_inst->GetSingleWordInOperand(1);
178205

179-
// Verify that the pointee type has the Block decoration, which is required for UBOs.
180-
// This ensures we don't accidentally convert a non-UBO uniform variable.
181-
if (!HasBlockDecoration(pointee_type_id))
182-
{
183-
// The struct type doesn't have Block decoration, not a valid UBO
184-
return Status::SuccessWithoutChange;
185-
}
186-
187206
// Create or find a pointer type with PushConstant storage class
188207
spvtools::opt::analysis::TypeManager* type_mgr = context()->get_type_mgr();
189208
uint32_t new_ptr_type_id =

0 commit comments

Comments
 (0)