Skip to content

Commit 9ce9ce6

Browse files
committed
[OPT] prevent private_to_local_pass optimizing double pointer
Fixes KhronosGroup#6156 before this fix the pass will optimize the following code: ``` %v = OpVariable %_ptr_Private_uint Private %uint_0 %load_thread_local = OpFunction %void None %function_type__1561_ %37 = OpLabel %31 = OpVariable %_ptr_Function__ptr_Private_uint Function %32 = OpVariable %_ptr_Function_uint Function %33 = OpVariable %_ptr_Function_uint Function OpStore %31 %v %34 = OpLoad %_ptr_Private_uint %31 %35 = OpLoad %uint %34 OpStore %32 %35 %36 = OpLoad %uint %v OpStore %33 %36 OpReturn OpFunctionEnd ``` to this: ``` %load_thread_local = OpFunction %void None %function_type__1561_ %37 = OpLabel %v = OpVariable %_ptr_Function_uint Function %uint_0 %31 = OpVariable %_ptr_Function__ptr_Private_uint Function %32 = OpVariable %_ptr_Function_uint Function %33 = OpVariable %_ptr_Function_uint Function OpStore %31 %v OpStore %32 %uint_0 OpStore %33 %uint_0 OpReturn OpFunctionEnd ``` resulting in the following error: ``` error: line 81: OpStore Pointer <id> '31[%31]'s type does not match Object <id> '47[%v]'s type. OpStore %31 %v ``` private_to_local_pass does not update the storage class of the double pointer %31 correctly. It currently doesn't have enough context to tell if %31 is later loaded from and used to store with. For now, just prevent the optimization if the address of the private variable is used as the object operand of the OpStore instruction. with this commit, the spirv code is optimized to this: ``` %v = OpVariable %_ptr_Private_uint Private %uint_0 %load_thread_local = OpFunction %void None %function_type__1561_ %37 = OpLabel %31 = OpVariable %_ptr_Function__ptr_Private_uint Function %32 = OpVariable %_ptr_Function_uint Function %33 = OpVariable %_ptr_Function_uint Function OpStore %31 %v %35 = OpLoad %uint %v OpStore %32 %35 %36 = OpLoad %uint %v OpStore %33 %36 OpReturn OpFunctionEnd ```
1 parent 0498065 commit 9ce9ce6

File tree

2 files changed

+8
-7
lines changed

2 files changed

+8
-7
lines changed

source/opt/private_to_local_pass.cpp

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,13 +90,13 @@ Function* PrivateToLocalPass::FindLocalFunction(const Instruction& inst) const {
9090
Function* target_function = nullptr;
9191
context()->get_def_use_mgr()->ForEachUser(
9292
inst.result_id(),
93-
[&target_function, &found_first_use, this](Instruction* use) {
93+
[&target_function, &found_first_use, inst, this](Instruction* use) {
9494
BasicBlock* current_block = context()->get_instr_block(use);
9595
if (current_block == nullptr) {
9696
return;
9797
}
9898

99-
if (!IsValidUse(use)) {
99+
if (!IsValidUse(use, inst.result_id())) {
100100
found_first_use = true;
101101
target_function = nullptr;
102102
return;
@@ -153,21 +153,22 @@ uint32_t PrivateToLocalPass::GetNewType(uint32_t old_type_id) {
153153
return new_type_id;
154154
}
155155

156-
bool PrivateToLocalPass::IsValidUse(const Instruction* inst) const {
156+
bool PrivateToLocalPass::IsValidUse(const Instruction* inst, uint32_t private_variable_id) const {
157157
// The cases in this switch have to match the cases in |UpdateUse|.
158158
// If we don't know how to update it, it is not valid.
159159
if (inst->GetCommonDebugOpcode() == CommonDebugInfoDebugGlobalVariable) {
160160
return true;
161161
}
162162
switch (inst->opcode()) {
163163
case spv::Op::OpLoad:
164-
case spv::Op::OpStore:
165164
case spv::Op::OpImageTexelPointer: // Treat like a load
166165
return true;
166+
case spv::Op::OpStore:
167+
return inst->GetOperand(1).AsId() != private_variable_id;
167168
case spv::Op::OpAccessChain:
168169
return context()->get_def_use_mgr()->WhileEachUser(
169-
inst, [this](const Instruction* user) {
170-
if (!IsValidUse(user)) return false;
170+
inst, [this, inst](const Instruction* user) {
171+
if (!IsValidUse(user, inst->result_id())) return false;
171172
return true;
172173
});
173174
case spv::Op::OpName:

source/opt/private_to_local_pass.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ class PrivateToLocalPass : public Pass {
5353
// Returns true is |inst| is a valid use of a pointer. In this case, a
5454
// valid use is one where the transformation is able to rewrite the type to
5555
// match a change in storage class of the original variable.
56-
bool IsValidUse(const Instruction* inst) const;
56+
bool IsValidUse(const Instruction* inst, uint32_t private_variable_id) const;
5757

5858
// Given the result id of a pointer type, |old_type_id|, this function
5959
// returns the id of a the same pointer type except the storage class has

0 commit comments

Comments
 (0)